Skip to content

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).

README.md Outdated
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)

README.md Outdated
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