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

Add regionCode test (manual and idl) #9706

Merged
merged 2 commits into from Mar 8, 2018

Conversation

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Feb 28, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots 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 marcoscaceres referenced this pull request Mar 1, 2018

Merged

Add regionCode attribute #690

4 of 6 tasks complete
@marcoscaceres

This comment has been minimized.

Copy link
Contributor Author

marcoscaceres commented Mar 8, 2018

@rsolomakhin, would like your ok on these.

@wpt-pr-bot wpt-pr-bot requested a review from mnoorenberghe Mar 8, 2018

@domenic

domenic approved these changes Mar 8, 2018

Copy link
Member

domenic left a comment

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

This comment has been minimized.

Copy link
Contributor Author

marcoscaceres commented Mar 8, 2018

np! thanks for checking them @domenic!

@rsolomakhin
Copy link
Contributor

rsolomakhin left a comment

Ping @sebsg to take a look.

region: '',
city: 'Kabul',
country: 'AU',
regionCode: 'AU-QLD',

This comment has been minimized.

Copy link
@rsolomakhin

rsolomakhin Mar 8, 2018

Contributor

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

This comment has been minimized.

Copy link
@marcoscaceres

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Mar 8, 2018

Author Contributor

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😭🇵🇹)

This comment has been minimized.

Copy link
@sebsg

sebsg Mar 8, 2018

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.

@rsolomakhin
Copy link
Contributor

rsolomakhin left a comment

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

@marcoscaceres

This comment has been minimized.

Copy link
Contributor Author

marcoscaceres commented Mar 8, 2018

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@marcoscaceres marcoscaceres deleted the regionCode branch Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.