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

Intl.Locale updates #4052

Merged
merged 17 commits into from
May 23, 2019
Merged

Intl.Locale updates #4052

merged 17 commits into from
May 23, 2019

Conversation

jahzielv
Copy link
Contributor

@jahzielv jahzielv commented May 2, 2019

A checklist to help your pull request get merged faster:

  • Summarize your changes
  • Data: link to resources that verify support information (such as browser's docs, changelogs, source control, bug trackers, and tests)
  • Data: if you tested something, describe how you tested with details like browser and version
  • Review the results of the linter and fix problems reported (If you need help, please ask in a comment!)
  • Link to related issues or pull requests, if any

This PR aims to add browser compat data for Intl.Locale and its various properties. This is part of a documentation effort involving Daniel Ehrenberg, Romulo Cintra, and myself. We're tracking our progress here.

npm test runs cleanly and Travis builds successfully.

According to my research, Chrome (desktop, Android, and WebView) is the only browser with support currently (shipped in Chrome 74). Firefox and Edge have issues open and are in development, but have not implemented yet. Safari hasn't issued any public signals regarding Intl.Locale.

Properties of Intl.Locale to add compat data for:

  • prototype
  • minimize()
  • maximize()
  • toString()
  • baseName
  • calendar
  • collation
  • hourCycle
  • caseFirst
  • numeric
  • numberingSystem
  • language
  • script
  • region
    Cheers!

@Elchi3 Elchi3 added the data:js Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript label May 2, 2019
@jahzielv
Copy link
Contributor Author

jahzielv commented May 6, 2019

Edit note: I added a list of all the properties of Intl.Locale and whether or not I've added compat data for them. I'll be adding data over the coming days!

@queengooborg
Copy link
Contributor

All the data is looking good to me! A little pro tip with Chromium browsers: since Opera 15 (and Opera Android 14), Opera adapted Chrome's engine into its own. For Opera desktop, the version numbers can be identified based upon the following formula: min(15, CHROME_VERSION - 13). (Opera Android was like that until version 43 -- I've included a mapping in my pull request to add engine versions). Samsung Internet's also Chrome-based, but for now, we just set it to true or false.

Regarding all the properties, would you like to add them in a single PR and wait for us to merge it until it's all done, or would you rather us merge this as it is and work on additional PRs?

@queengooborg queengooborg added the info needed This needs more information to review or act on. label May 6, 2019
@jahzielv
Copy link
Contributor Author

jahzielv commented May 6, 2019

Thanks for the tips @vinyldarkscratch! As far as the properties go, I think I'll add them all on this PR and then we can merge them all at once. I'll be offline this week, but should have some time Saturday to get back on this. Thanks!

@queengooborg
Copy link
Contributor

Alright, sounds like a plan! I’ll mark this as “not ready” for the time being. 😉

@queengooborg queengooborg added not ready This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. and removed info needed This needs more information to review or act on. labels May 6, 2019
@jahzielv
Copy link
Contributor Author

@vinyldarkscratch regarding your tip about Opera: Taking a look at this version history, it seems like the latest version (Opera 60) uses Chromium 73, which wouldn't support Intl.Locale. Could you elaborate a bit on what you meant by

For Opera desktop, the version numbers can be identified based upon the following formula: min(15, CHROME_VERSION - 13). (Opera Android was like that until version 43 -- I've included a mapping in my pull request to add engine versions). Samsung Internet's also Chrome-based, but for now, we just set it to true or false.

? Thanks for your help with this!

@queengooborg
Copy link
Contributor

Sorry, you can totally ignore that part! I wrote that before I found out Opera 61 and 62 are only beta versions. 😛

@jahzielv
Copy link
Contributor Author

Cool cool thanks!

@jahzielv
Copy link
Contributor Author

All data has been added! npm test runs cleanly, but I have a question about the rendering. When I run a render test, the resulting table looks like this:

Screen Shot 2019-05-18 at 12 03 15 AM

The data is correct, but the table isn't styled correctly. Am I missing something in the command? I ran

npm run render javascript.builtins.Intl.Locale.script

@queengooborg queengooborg removed the not ready This is not yet ready to be merged. It's pending a decision, other PR, or a prerequisite action. label May 18, 2019
@queengooborg
Copy link
Contributor

Don't mind the old styling with npm run render, efforts to get it to use the new styling sheet are in #736 and #737. Thanks for your hard work on this PR! I'll be reviewing it shortly. 😉

@queengooborg queengooborg self-requested a review May 18, 2019 04:49
@jahzielv jahzielv changed the title Intl local updates Intl.Locale updates May 21, 2019
Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

All in all, looks good! Just one small change to make.

@jahzielv
Copy link
Contributor Author

Order has been updated! 🎉

Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Looking good to me, thank you! Just made the change to the wiki that will add this BCD in as well in the next Kumascript release. 😉

@queengooborg queengooborg merged commit fa4f4af into mdn:master May 23, 2019
@jahzielv
Copy link
Contributor Author

Woot woot - thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:js Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants