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

Interactive example for Intl.Locale #1394

Merged
merged 11 commits into from
Jul 20, 2019
Merged

Conversation

jahzielv
Copy link
Contributor

Added an interactive example for Intl.Locale. Demonstrates several of Locale's properties.

Example renders correctly and outputs correct values.

Part of an MDN documentation effort for Intl.Locale on behalf of ECMA 402.

Thanks!

Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks @jahzielv ! It seems like lot of comments but they're all little formatting things.

@@ -0,0 +1,12 @@
<pre>
<code id="static-js">const korean = new Intl.Locale("ko", {script: "Kore", region: "KR", hourCycle: "h24", calendar: "gregory"});
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than have it on one line it would be easier to read like:

const korean = new Intl.Locale("ko", {
  script: "Kore", region: "KR", hourCycle: "h24", calendar: "gregory"
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Also here and elsewhere, single quotes for strings is our style.
(although note: use double for the expected output strings)

@@ -0,0 +1,12 @@
<pre>
<code id="static-js">const korean = new Intl.Locale("ko", {script: "Kore", region: "KR", hourCycle: "h24", calendar: "gregory"});
const japan = new Intl.Locale("ja-Jpan-JP-u-ca-japanese-hc-h12")
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere, end the line with a semicolon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we should be consistent with variable names: so either:

"japan" and "korea"
or
"japanese" and "korean"

@@ -0,0 +1,12 @@
<pre>
<code id="static-js">const korean = new Intl.Locale("ko", {script: "Kore", region: "KR", hourCycle: "h24", calendar: "gregory"});
const japan = new Intl.Locale("ja-Jpan-JP-u-ca-japanese-hc-h12")
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a blank space before this line (to separate the two declarations) would help readability I think.

// expected output: "ko-Kore-KR"

console.log(japan.baseName)
// expected output: "ja-Jpan-JP"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since my above comments make the example longer, why not compress this bit as you do below, with something like:

console.log(korean.baseName, japan.baseName)
// expected output: "ko-Kore-KR" "ja-Jpan-JP"

<pre>
<code id="static-js">const korean = new Intl.Locale("ko", {script: "Kore", region: "KR", hourCycle: "h24", calendar: "gregory"});
const japan = new Intl.Locale("ja-Jpan-JP-u-ca-japanese-hc-h12")
console.log(korean.baseName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a blank line before this (after the 2 declarations) would help readability.

@jahzielv
Copy link
Contributor Author

Requested changes made! :D

Copy link
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Looks great @jahzielv . Thanks again for your contributions!

@wbamberg wbamberg merged commit 393d820 into mdn:master Jul 20, 2019
@jahzielv
Copy link
Contributor Author

Thanks @wbamberg! My pleasure :D

@jahzielv jahzielv deleted the jve-intl-general branch July 20, 2019 02:20
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.

2 participants