-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: update synopsis, toc, other nits #6167
Conversation
LGTM |
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. |
👍 |
@@ -1,7 +1,10 @@ | |||
@// NB(chrisdickinson): if you move this file, be sure to update tools/doc/html.js to | |||
@// point at the new location. | |||
* [About these Docs](documentation.html) | |||
* [Synopsis](synopsis.html) | |||
* [Synopsis & Example](synopsis.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Two nits, generally LGTM |
@@ -13,7 +13,7 @@ | |||
<div id="column2" class="interior"> | |||
<div id="intro" class="interior"> | |||
<a href="/" title="Go back to the home page"> | |||
Node.js (1) | |||
Node.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more appropriate to be NODE(1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nm. wasn't paying attention to what file this was located in. our man page should be NODE(1)
, but this looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our man page is NODE(1)
since I rewrote it. :P
e561107
to
2953a06
Compare
@benjamingr Updated, ptal. @bengl have time to take a look? |
As a note, I didn't rename the filename for |
@@ -1,29 +1,43 @@ | |||
# Synopsis | |||
# Usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
7da4fd4
to
c7066fb
Compare
@nodejs/documentation could we get some more opinions here about the file name/title thing? |
Also, if we can leave that for a later PR, that would be great; I'd really like to get some of this merged. |
2953a06
to
b70e5d6
Compare
Please see the [Command Line Options][] document for information about | ||
different options and ways to run scripts with Node. | ||
|
||
## Hello World Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer Hello World
or "Hello World!"
or nothing at all.
LGTM w/ nits. |
b70e5d6
to
ec46ff1
Compare
Updated, |
👍 done deal then. |
LGTM |
``` | ||
|
||
To run the server, put the code into a file called `example.js` and execute | ||
it with the node program | ||
it with Node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/Node/Node.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better as node example.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that works too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, that's on the lines below
ec46ff1
to
a4c4e58
Compare
nodejs#6167 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
Node.js(1) does not make sense. Node(1) would, but this isn’t a `man` page. nodejs#6167 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
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 |
@thealphanerd This lands cleanly for me on |
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
@Fishrock123 must have caught the missing bits in other backports yesterday. thanks for checking |
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
PR-URL: #6167 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]>
Checklist
Affected core subsystem(s)
doc
Description of change
There are a few things here:
Synopsis
toUsage & Example
Node.js(1)
in the TOC is nowNode.js
since the former does not make sense in any contextcc @nodejs/documentation