Skip to content

Add regionCode test (manual and idl)#9706

Merged
marcoscaceres merged 2 commits intomasterfrom
regionCode
Mar 8, 2018
Merged

Add regionCode test (manual and idl)#9706
marcoscaceres merged 2 commits intomasterfrom
regionCode

Conversation

@marcoscaceres
Copy link
Contributor

@marcoscaceres marcoscaceres commented Feb 28, 2018

@ghost
Copy link

ghost commented Feb 28, 2018

Build PASSED

Started: 2018-02-28 06:21:35
Finished: 2018-02-28 06:31:40

View more information about this build on:

@marcoscaceres
Copy link
Contributor Author

@rsolomakhin, would like your ok on these.

@wpt-pr-bot wpt-pr-bot requested a review from mnoorenberghe March 8, 2018 04:16
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM although @rsolomakhin would be in a better position to confirm that this exactly matches Chrome's user input -> regionCode translation algorithm.

EDIT: oops, I totally misread my email and thought you were asking for me. Yes, please, let's have @rsolomakhin confirm :)

@marcoscaceres
Copy link
Contributor Author

np! thanks for checking them @domenic!

Copy link
Contributor

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

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

Ping @sebsg to take a look.

region: '',
city: 'Kabul',
country: 'AU',
regionCode: 'AU-QLD',
Copy link
Contributor

Choose a reason for hiding this comment

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

@sebsg: Is this what Australian region codes look like? (I expected only QLD without AU-, but you know better.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check out the Portuguese ones... they are numbered. Super interesting: https://en.m.wikipedia.org/wiki/ISO_3166-2:PT

(I was going to use Portuguese ones originally, but they gave too much “saudade” 😭🇵🇹)

Copy link
Contributor

@sebsg sebsg Mar 8, 2018

Choose a reason for hiding this comment

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

I was under that impression too, but it seems like it's like that everywhere. "US-CA" for example. I think it's because a few use numbers like marcoscaceres mentionned and they need to be unique.

Copy link
Contributor

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

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

LGTM. Although we store without AU- prefix internally, we can add the <country-code>- prefix for all region codes.

@marcoscaceres
Copy link
Contributor Author

Although we store without AU- prefix internally, we can add the - prefix for all region codes.

Sounds good as long as it conforms to the ISO standard.

@marcoscaceres marcoscaceres merged commit da1dbe3 into master Mar 8, 2018
@marcoscaceres marcoscaceres deleted the regionCode branch March 21, 2018 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants