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

Transform country name to country code upon submitting finish_sign_up… #24993

Merged
merged 3 commits into from Sep 25, 2018

Conversation

maddiedierker
Copy link
Contributor

… form

The value of the country set by <CountryAutocompleteDropdown/> is the full string name of the country chosen from the dropdown, but we want the country to actually be the country's 2-letter country code upon submitting the signup form.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Test?

@maddiedierker
Copy link
Contributor Author

@islemaster my current plan is to write UI tests for this functionality once validation is in place. is this okay, or do you think there should be unit test(s) for this as well?

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

That should be okay. Thanks!

@breville
Copy link
Member

It feels slightly fragile to do a lookup based on the user-facing name, especially if we decide to localise this list at some point.

One alternate approach is that in the onCountryChange handler, you can also store the label along with the value, and the label will be the country code you're looking to use here.

@maddiedierker maddiedierker merged commit a32ff14 into staging Sep 25, 2018
@maddiedierker maddiedierker deleted the country-to-country-code branch September 25, 2018 19:03
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.

None yet

3 participants