Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSL-12 - PCA - Added new pages companies-house-number and companies-house-name #52

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vinodhasamiyappanHO
Copy link
Contributor

What?

CSL-12 - Adds the configuration and files required to display the "companies-house-number and companies-house-name" pages

How?

  • Added necessary step configuration and field specific variables.
  • Added required field validation for all pages

Testing?

Screenshots (optional)

Anything Else? (optional)

Check list

  • I have reviewed my own pull request for linting issues (e.g. adding new lines)
  • I have written tests (if relevant)
  • I have created a JIRA number for my branch
  • I have created a JIRA number for my commit
  • I have followed the chris beams method for my commit https://cbea.ms/git-commit/
    here is an example commit
  • Ensure drone builds are green especially tests
  • I will squash the commits before merging

Comment on lines 467 to +474
'declaration-check': {
mixin: 'checkbox',
validate: ['required']
},
'companies-house-number': {
isPageHeading: 'true',
mixin: 'radio-group',
validate: ['required'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose placing fields related to the two other flows before the main flow, as they occur first if you make choices so you have to fill them in. I suppose that means that for each file in this PR where something is added here it should go at the top of the list instead of the bottom

What do you think @dk4g @vinodhasamiyappanHO @RobertMcCann

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree on this, this will help with quick navigation

@@ -0,0 +1,5 @@
{{<partials-page}}
{{$page-content}}
<a href="/" class="govuk-link">Register for the licensing platform again.</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Not sure how the registration design looks yet, but I suppose this href could change depending on the routes - e.g. it might go to /registration.
  2. Can we move the link text into the translations for this field?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the correct link is /registration
The registration form will have a landing page /registration/ with subsequent pages with the same prefix /registration/<page>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link text should be moved in translations, I would suggest to use buttons.json

@@ -0,0 +1,5 @@
{{<partials-page}}
{{$page-content}}
<a href="/" class="govuk-link">Register for the licensing platform again.</a>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the correct link is /registration
The registration form will have a landing page /registration/ with subsequent pages with the same prefix /registration/<page>

@@ -0,0 +1,5 @@
{{<partials-page}}
{{$page-content}}
<a href="/" class="govuk-link">Register for the licensing platform again.</a>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link text should be moved in translations, I would suggest to use buttons.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants