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

Support uppercase hash override in amp-geo. #15532

Merged
merged 5 commits into from May 24, 2018

Conversation

gmajoulet
Copy link
Contributor

Support uppercase hash override in amp-geo to make sure both options work:

  • https://foo.bar#amp-geo=de
  • https://foo.bar#amp-geo=DE

@jpettitt
Copy link
Contributor

jpettitt commented May 23, 2018

I don't really like this. The problem I have with it is the countries specified in the json config must be lower case. Allowing upper case here when we don't allow it in the data we're matching is likely to cause confusion. The doc is clear that it's the lowercase ISO 3166-1 alpha-2 country code.

Why is it needed?

@gmajoulet
Copy link
Contributor Author

It is not needed, I spent a few minutes thinking my implementation of the component was wrong, when I actually was using an uppercase country code. Thought I could help someone.
Being consistent makes a lot more sense, thanks for the review.

@gmajoulet gmajoulet closed this May 23, 2018
@jpettitt
Copy link
Contributor

It might make sense if you also added

ISOCountryGroups[group] = ISOCountryGroups[group].map(country => country.toLowerCase());

in matchCountryGroups_ that would force the config to lower case too which would make it more robust against users using an uppercase config by mistake.

@jpettitt jpettitt reopened this May 23, 2018
Copy link
Contributor

@jpettitt jpettitt left a comment

Choose a reason for hiding this comment

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

LGTM

@gmajoulet gmajoulet merged commit cfe791a into ampproject:master May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants