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

doc: add npm link and cleanup Readme #7894

Closed
wants to merge 1 commit into from

Conversation

oscarmorrison
Copy link
Contributor

Note: this is a cleaned (single commit) of the PR #7769

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

README.md missing npm link
formatted links for consistency and readability

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 27, 2016
@addaleax
Copy link
Member

LGTM (and for whoever lands this, there already are reviews in #7769)

@oscarmorrison
Copy link
Contributor Author

Hi @addaleax @Fishrock123 @cjihrig @Trott @ChALkeR,
I decided to make it a single commit off the current master.
Old Ref: PR #7769

Is it ok to go?

@jasnell
Copy link
Member

jasnell commented Jul 29, 2016

LGTM

Just an fyi @oscarmorrison ... it most cases, multiple commits in a single PR can be "squashed" into a single commit without requiring a new PR to be opened. This can be done using git rebase -i (the -i is for "interactive" mode).

* [Website][]
* [Contributing to the project][]
* [nodejs.org](https://nodejs.org/)
* Contributing: [CONTRIBUTING.md](./CONTRIBUTING.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change would leave two dead links at the bottom of the page. I'd prefer if you could just update the links on the bottom instead. The relative link to CONTRIBUTING.md could be problematic, how about just adding a link to the file on master instead?

@lpinca
Copy link
Member

lpinca commented Sep 25, 2016

I agree with @yorkie and @silverwind about the changed links. I think it would be better to just define them at the bottom.
Apart from this LGTM.

@oscarmorrison can you please rebase this against master? Thanks!

@oscarmorrison
Copy link
Contributor Author

@lpinca You think I would remove the https://www.npmjs.com link?

@lpinca
Copy link
Member

lpinca commented Sep 26, 2016

No, I don't think so I would keep it for now.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@oscarmorrison oscarmorrison force-pushed the patch-2 branch 2 times, most recently from e8e52c5 to 590bc47 Compare November 9, 2016 03:47
@oscarmorrison
Copy link
Contributor Author

@lpinca @jasnell @addaleax
can we get this out. I have rebased against master, and removed the changes, as no longer needed(since there has been some readme changes since this original PR)

@@ -4,7 +4,7 @@

Node.js is a JavaScript runtime built on Chrome's V8 JavaScript engine. Node.js
uses an event-driven, non-blocking I/O model that makes it lightweight and
efficient. The Node.js package ecosystem, npm, is the largest ecosystem of open
efficient. The Node.js package ecosystem, [npm][], is the largest ecosystem of open
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this line is too long, can you please wrap it a 80 chars? Thanks.

missing the npm link in the readme.

wrap line
@oscarmorrison
Copy link
Contributor Author

Thanks @lpinca i have made change and squashed commit.

@silverwind
Copy link
Contributor

Thanks! Landed in d548d28.

@silverwind silverwind closed this Nov 9, 2016
silverwind pushed a commit that referenced this pull request Nov 9, 2016
PR-URL: #7894
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 22, 2016
PR-URL: #7894
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
PR-URL: #7894
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2016
PR-URL: #7894
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
PR-URL: #7894
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
PR-URL: #7894
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants