-
Notifications
You must be signed in to change notification settings - Fork 69
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
Show "postal code check" label based on store country #9952
base: develop
Are you sure you want to change the base?
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: -89 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
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.
Capitalization changes look fine, but I'm not sure I agree that this exact set of changes is necessary, see discussion :)
/** | ||
* Returns the label for the postal code check. | ||
* | ||
* @return {string} The label for the postal code check. | ||
*/ | ||
const getPostalCodeCheckLabel = () => { | ||
switch ( wcpaySettings.connect.country ) { | ||
case 'US': | ||
return 'ZIP check'; | ||
case 'UK': | ||
return 'Postcode check'; | ||
default: | ||
return __( 'Postal code check', 'woocommerce-payments' ); | ||
} | ||
}; | ||
|
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.
Changes the "postal code check" label to display dynamically based on the store country. I chose that approach over just localizing "Postal code check" because that leaves it open to the translator to decide how to translate.
I think this should be left up to the translator. This is just part of the knowledge you need to have if you're doing translations. For example, I know that in Canada a "ZIP code" is typically called a "postal code", and in Icelandic it's "póstnúmer" (e. post number).
If we're worried that the addition of "check" creates some confusion for translators I think it'd be better to add a translation note with some additional context like Label for results of a ZIP check performed by a credit card issuer
, or something along those lines instead of trying to localize this ourselves for some countries but not others.
FYI @aheckler since I think you reported the original issue? I think "Postal code check" will be just as confusing for a US person as a "ZIP code check" will be for a UK/CA person. I think this is best left to localizations instead of trying to maintain this ourselves.
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.
This is just part of the knowledge you need to have if you're doing translations
I went down that path initially because anyone can contribute to translations. But I agree with your other points, especially about maintenance, and that's why I adopted your suggestion for reverting that and adding a translator note in 85b041d 👍
@media screen and ( max-width: 470px ) { | ||
flex: 0 0 40%; | ||
} |
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 included this change here because "Postal code check" does not look good in some mobile devices. You can test this change in a "iPhone 14 Pro Max" dimension.
Fixes #9896.
Changes proposed in this Pull Request
This PR does two things:
Testing instructions
Test: Ensure the postal code check label changes based on the store country
npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge