FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: add documentation on ICU by TimothyGu · Pull Request #13916 · nodejs/node · GitHub

/ node Public

doc: add documentation on ICU - #13916

Closed
TimothyGu wants to merge 6 commits into
nodejs:masterfrom
TimothyGu:intl-doc
ConversationCommits (6)ChecksFiles changed
Closed

doc: add documentation on ICU#13916
TimothyGu wants to merge 6 commits into
nodejs:masterfrom
TimothyGu:intl-doc

Conversation

TimothyGu commented Jun 26, 2017

Copy link
Copy Markdown
Member

Adapted from the wiki article "Intl" by @srl295. Open to suggestions w.r.t. where this page belongs best, either here or on nodejs.org, but somewhere more permanent than a wiki.

Refs: #13644 (comment)

Checklist
Affected core subsystem(s)

doc

TimothyGu requested a review from srl295 June 26, 2017 05:12
nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 26, 2017
TimothyGu added the i18n-api Issues and PRs related to the i18n implementation. label Jun 26, 2017

vsemozhetbyt commented Jun 26, 2017

Copy link
Copy Markdown
Contributor

If it remains in the api, it should also be added to all.md

vsemozhetbyt commented Jun 26, 2017

Copy link
Copy Markdown
Contributor

Linter CI: https://ci.nodejs.org/job/node-test-linter/10073/

jasnell reviewed Jun 26, 2017
View reviewed changes
Comment thread doc/api/intl.md Outdated

jasnell Jun 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please avoid use of informal pronouns like we in the docs... Also, this last sentence is a bit off for me... Perhaps:

Because it is expected that most Node.js users will make use of only a small portion
of ICU functionality, only a subset of the full ICU data set is provided by Node.js by
default. Several options are provided for customizing and expanding the ICU data set
either when building or running Node.js.

jasnell reviewed Jun 26, 2017
View reviewed changes
Comment thread doc/api/intl.md Outdated

jasnell Jun 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please avoid using informal pronouns like your in the docs.

jasnell reviewed Jun 26, 2017
View reviewed changes
Comment thread doc/api/intl.md Outdated

jasnell Jun 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As above, please avoid using informal pronouns like you

TimothyGu commented Jun 27, 2017

Copy link
Copy Markdown
Member Author

@jasnell Comments are addressed. What are your thoughts on where this document should end up?

TimothyGu commented Jul 5, 2017

Copy link
Copy Markdown
Member Author

I'll merge this tomorrow if there are no more comments.

vsemozhetbyt commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

Linter (you may need to rebase after the 15599cb):

  96:1  error  Expected indentation of 0 spaces but found 2  indent
  98:1  error  Expected indentation of 0 spaces but found 2  indent
  99:1  error  Expected indentation of 0 spaces but found 2  indent

TimothyGu commented Jul 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Not too big of a fan of the linter change, but changed.

Linter CI: https://ci.nodejs.org/job/node-test-linter/10274/

vsemozhetbyt commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

@TimothyGu It seems it is due to 7477f9b, but I wanted to be on safe side with updated tools)

TimothyGu commented Jul 6, 2017

Copy link
Copy Markdown
Member Author

Landed in 6809429

TimothyGu closed this Jul 6, 2017
TimothyGu deleted the intl-doc branch July 6, 2017 03:21
TimothyGu added a commit that referenced this pull request Jul 6, 2017
doc: add documentation on ICU
6809429 
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
doc: add documentation on ICU
48843bd 
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
doc: add documentation on ICU
9e6a4d6 
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
doc: add documentation on ICU
7894578 
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

MylesBorins commented Aug 15, 2017

Copy link
Copy Markdown
Contributor

Is this applicable to v6.x? It is easy enough to land this but I want to ensure that it makes sense for the version of ICU that is embedded

6.x: 58.2
8.x: 59.1

TimothyGu commented Aug 15, 2017

Copy link
Copy Markdown
Member Author

@MylesBorins Yep, most of the content is applicable to 6.x as well, but some things need to be changed (e.g. mentions of the WHATWG URL API as it doesn't exist in 6.x).

srl295 reviewed Aug 31, 2017
View reviewed changes

srl295 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

post merge 👍

TimothyGu mentioned this pull request Sep 12, 2017
Closed
2 tasks
TimothyGu added a commit to TimothyGu/node that referenced this pull request Sep 15, 2017
doc: add documentation on ICU
d7293be 
PR-URL: nodejs#13916
Refs: nodejs#13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
doc: add documentation on ICU
90fcccd 
PR-URL: #13916
Refs: #13644 (comment)
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
MylesBorins mentioned this pull request Sep 20, 2017
Merged
TimothyGu mentioned this pull request Dec 17, 2017
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
srl295 srl295 left review comments
+1 more reviewer
vsemozhetbyt vsemozhetbyt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. i18n-api Issues and PRs related to the i18n implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL