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

doc: update synopsis, toc, other nits by Fishrock123 · Pull Request #6167 · nodejs/node · GitHub

/ node Public

doc: update synopsis, toc, other nits - #6167

Merged
Fishrock123 merged 3 commits into
nodejs:masterfrom
Fishrock123:modernize-docs
Apr 29, 2016
ConversationCommits (3)ChecksFiles changed
Merged

doc: update synopsis, toc, other nits#6167
Fishrock123 merged 3 commits into
nodejs:masterfrom
Fishrock123:modernize-docs

Conversation

Fishrock123 commented Apr 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • tests and code linting passes
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

There are a few things here:

  • Updated the Example to be more modern and better reflect the example in https://nodejs.org/en/about/
  • Renamed Synopsis to Usage & Example
  • Node.js(1) in the TOC is now Node.js since the former does not make sense in any context
  • The TOC is now subdivided: general about, subsystems, extra links
  • Added a toc / sidebar link to the repo/issue tracker and also the mailing list

cc @nodejs/documentation

Fishrock123 added the doc Issues and PRs related to the documentations. label Apr 12, 2016
Fishrock123 changed the title Modernize docs doc: update synopsis, toc, other nits Apr 12, 2016

jasnell commented Apr 12, 2016

Copy link
Copy Markdown
Member

LGTM

Fishrock123 commented Apr 12, 2016

Copy link
Copy Markdown
Contributor Author

Here's some rendering of the toc subdivisions, tested on chrome, ff, safari:

Fishrock123 commented Apr 12, 2016

Copy link
Copy Markdown
Contributor Author

Hmm, looks like I wasn't quite accurate with those shots. The lines do have the same relative margin on both sides in the sidebar.

Knighton910 commented Apr 12, 2016

Copy link
Copy Markdown

👍

benjamingr reviewed Apr 12, 2016
View reviewed changes
Comment thread doc/api/_toc.markdown Outdated

benjamingr Apr 12, 2016

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

I'm not sure about that

  • That file is not a synopsis of anything.
  • Example doesn't really describe much.

What about "Hello World" or "Usage" or something like that?

benjamingr commented Apr 12, 2016

Copy link
Copy Markdown
Member

Two nits, generally LGTM

trevnorris reviewed Apr 12, 2016
View reviewed changes
Comment thread doc/template.html Outdated

trevnorris Apr 12, 2016

Copy link
Copy Markdown
Contributor

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

It would be more appropriate to be NODE(1).

trevnorris Apr 12, 2016

Copy link
Copy Markdown
Contributor

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

nm. wasn't paying attention to what file this was located in. our man page should be NODE(1), but this looks good.

Fishrock123 Apr 12, 2016

Copy link
Copy Markdown
Contributor Author

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

Our man page is NODE(1) since I rewrote it. :P

Fishrock123 commented Apr 15, 2016

Copy link
Copy Markdown
Contributor Author

@benjamingr Updated, ptal.

@bengl have time to take a look?

Fishrock123 commented Apr 15, 2016

Copy link
Copy Markdown
Contributor Author

As a note, I didn't rename the filename for synopsis.markdown since I didn't really want to bikeshed it here, but perhaps intro is a better name for the file.

bengl reviewed Apr 15, 2016
View reviewed changes
Comment thread doc/api/synopsis.markdown Outdated

bengl Apr 15, 2016 •
edited
Loading

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

The title being different from the filename is potentially concerning, as mentioned already in a comment.

Could the filename be changed, and then a redirect added in synopsis.html to preserve links?

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

Fishrock123 commented Apr 27, 2016

Copy link
Copy Markdown
Contributor Author

@nodejs/documentation could we get some more opinions here about the file name/title thing?

Fishrock123 commented Apr 27, 2016

Copy link
Copy Markdown
Contributor Author

Also, if we can leave that for a later PR, that would be great; I'd really like to get some of this merged.

eljefedelrodeodeljefe reviewed Apr 27, 2016
View reviewed changes
Comment thread doc/api/synopsis.md Outdated

eljefedelrodeodeljefe Apr 27, 2016

Copy link
Copy Markdown
Contributor

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

I'd prefer Hello World or "Hello World!" or nothing at all.

eljefedelrodeodeljefe commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

LGTM w/ nits.

Fishrock123 commented Apr 27, 2016

Copy link
Copy Markdown
Contributor Author

Updated, ## Hello World Example is now just ## Example.

eljefedelrodeodeljefe commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

👍 done deal then.

kahwee commented Apr 27, 2016

Copy link
Copy Markdown

LGTM

jasnell reviewed Apr 29, 2016
View reviewed changes
Comment thread doc/api/synopsis.md Outdated

jasnell Apr 29, 2016

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

nit: s/Node/Node.js

Fishrock123 Apr 29, 2016

Copy link
Copy Markdown
Contributor Author

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

Maybe better as node example.js?

jasnell Apr 29, 2016

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

that works too

Fishrock123 Apr 29, 2016

Copy link
Copy Markdown
Contributor Author

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

Nvm, that's on the lines below

doc: better example & synopsis
2595a7c 
nodejs#6167
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
doc: no Node.js(1)
3a4224d 
Node.js(1) does not make sense.
Node(1) would, but this isn’t a `man` page.

nodejs#6167
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>

MylesBorins commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

looks like this relies on some changes not in v4.x

@Fishrock123 please feel free to send a pr if you want to backport this

Fishrock123 commented Jun 2, 2016

Copy link
Copy Markdown
Contributor Author

@thealphanerd This lands cleanly for me on v4.x-staging.

MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: better example & synopsis
a4888b0 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: no Node.js(1)
fb14bc9 
Node.js(1) does not make sense.
Node(1) would, but this isn’t a `man` page.

PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: subdivide TOC, add auxiliary links
3001f88 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>

MylesBorins commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

@Fishrock123 must have caught the missing bits in other backports yesterday. thanks for checking

MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: better example & synopsis
37daa32 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: no Node.js(1)
124dfc1 
Node.js(1) does not make sense.
Node(1) would, but this isn’t a `man` page.

PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: subdivide TOC, add auxiliary links
a87269d 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins mentioned this pull request Jun 24, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: better example & synopsis
401325f 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: no Node.js(1)
107839c 
Node.js(1) does not make sense.
Node(1) would, but this isn’t a `man` page.

PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: subdivide TOC, add auxiliary links
eadb7e5 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: better example & synopsis
d3c7b58 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: no Node.js(1)
f7068d4 
Node.js(1) does not make sense.
Node(1) would, but this isn’t a `man` page.

PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: subdivide TOC, add auxiliary links
3189446 
PR-URL: #6167
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Jefe Lindstädt <robert.lindstaedt@gmail.com>
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

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL