-
Notifications
You must be signed in to change notification settings - Fork 12
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
PP-11605 Update billing address section #3775
Conversation
416a120
to
c41d157
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and thanks for including screenshots. I have a couple queries rather than requested changes.
// body is passed by reference | ||
const addressLines = function (body) { | ||
if (!body.addressLine1 && body.addressLine2) { | ||
body.addressLine1 = body.addressLine2 | ||
delete body.addressLine2 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is:
Remove code that would combine address line 1 & 2 together. This code is no longer
required because each line now has its own field label.
However, it looks like that’s not quite what it was doing. What would happen was if there was no text for address line 1 but there was for address line 2, it would shift the address line 2 text up to address line 1. We’re losing that behaviour now. That might be fine but I want to check we understand what we’re doing and why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - we removed this code as I didn't want address line 2
shifting to address line 1
anymore.
As we now have validation for address line 1
and address line 2
individually.
locales/cy.json
Outdated
"name": "enw/rhif yr adeilad a heol", | ||
"message": "Rhowch gyfeiriad bilio dilys", | ||
"containsTooManyDigits": "Rhowch gyfeiriad bilio dilys" | ||
"name": "Llinell cyfeiriad 1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t know Welsh but I note this begins with a capital letter while the English equivalent does not. Should these be different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot - just updated it
locales/cy.json
Outdated
"name": "llinell cyfeiriad ychwanegol", | ||
"message": "Rhowch wybodaeth cyfeiriad dilys", | ||
"containsTooManyDigits": "Rhowch wybodaeth cyfeiriad dilys" | ||
"name": "Llinell cyfeiriad 2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same initial letter difference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot - just updated it.
"containsTooManyDigits": "Enter valid address information" | ||
"name": "address line 2", | ||
"message": "Enter address line 2", | ||
"containsTooManyDigits": "Enter address line 2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m a bit concerned that this does not really tell people what the problem is (too many digits). It might trip people up entering foreign addresses. For example, in Germany, the postcode is written immediately before the city so it’s conceivable that someone could put it in the address line 2 input because that’s the one before the city… and German postcodes are all digits (but only five digits, so not enough to trigger our card number check). But since this is an existing issue I’m not going to hold up the review on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I didn't realise German postcodes work like that.
We noticed a few minor existing bugs with the validation and raised separate tickets for them.
- Making the billing address section more consistent like the guidelines from design system https://design-system.service.gov.uk/patterns/addresses/ - Address line 2 now has its own label - This field is still optional. - Validation continues to only happen to this field when it contains a value. - Remove code that would combine address line 1 & 2 together. This code is no longer required because each line now has its own field label. - Moved the country select to above the postcode field. - We can't make our page exacly like the design systems guidelines. As we need the user to select the country before entering the post code. So that we know which rules to apply to the post code validation. - Decision taken to move the country select just above the post code field - Updated the translations - There is any existing issue where the error summary does not display errors in the correct order. A seperate story was created for this. - Updated the tests and removed old tests that are not required.
c41d157
to
c5b897a
Compare
https://design-system.service.gov.uk/patterns/addresses/
value.
required because each line now has its own field label.
the user to select the country before entering the post code. So that we know
which rules to apply to the post code validation.
order. A seperate story was created for this.
New address design - based on latest design system guidelines
Address line 2 has its own label:
Does not show validation error for
address line 2
if it is empty, as it is an optional fieldIf address line 2 contains an invalid entry, then show a validation error