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 to Readme, general cleanup #7769

Closed
wants to merge 4 commits into from

Conversation

oscarmorrison
Copy link
Contributor

@oscarmorrison oscarmorrison commented Jul 16, 2016

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

README.md missing npm link
removed unnecessary comma
formatted links for consistency and readability
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 16, 2016
* [Website](https://nodejs.org/en/)
* [Contributing to the project](./CONTRIBUTING.md)
* Website: [nodejs.org](https://nodejs.org/en/)
* Project Contributing: [CONTRIBUTING.md](./CONTRIBUTING.md)
Copy link
Member

Choose a reason for hiding this comment

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

I am not a native English speaker, but «Project Contributing» does sound a little suspicious to me.
/cc @nodejs/documentation

removed unnecessary comma
formatted links for consistency and readability
@@ -5,8 +5,8 @@ Node.js

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
source libraries in the world.
efficient. The Node.js package ecosystem [npm](https://github.com/npm/npm),
Copy link
Contributor

Choose a reason for hiding this comment

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

The comma before npm should not be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the link should rather go to https://www.npmjs.com/?

@oscarmorrison
Copy link
Contributor Author

Thanks @ChALkeR updated based on your feedback

@oscarmorrison
Copy link
Contributor Author

Updated doc based on feedback from
@cjihrig and @addaleax
Thanks

@@ -19,8 +19,8 @@ If you need help using or installing Node.js, please use the

## Resources for Newcomers

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

Choose a reason for hiding this comment

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

I know this wasn't part of this PR, but does anyone know why we are sending everyone to /en?

Copy link
Member

Choose a reason for hiding this comment

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

Ref: #6681, /cc @Fishrock123.

Copy link
Contributor

Choose a reason for hiding this comment

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

No reason, it was a copy-paste of what the default is.

This should be fixed.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 18, 2016

npm link lgtm but I'm not sure about the other stuff and I would rather keep the newcomer links not verbose.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 18, 2016

LGTM either way once the /en is dropped from the URL.

@oscarmorrison
Copy link
Contributor Author

How about just as: [nodejs.org](https://nodejs.org)
@Fishrock123?
I find Website weird.

@Trott
Copy link
Member

Trott commented Jul 19, 2016

How about just as: [nodejs.org](https://nodejs.org)

Nitpick (ignore if you wish) but I believe a trailing slash is more correct here:

[node.js.org](https://nodejs.org/)

@oscarmorrison
Copy link
Contributor Author

Updated based on feedback
@cjihrig @Fishrock123
thanks

@oscarmorrison
Copy link
Contributor Author

Ready to go? @Fishrock123 @cjihrig @Trott @ChALkeR

@thefourtheye
Copy link
Contributor

LGTM.

@addaleax
Copy link
Member

Sorry to ask again, but could you rebase this?

@oscarmorrison
Copy link
Contributor Author

I have closed this PR for a single commit branched from current master:
#7894

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