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.

Sign up for GitHub

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

Jump to bottom

doc: add recommendations for first timers #16350

Merged
merged 1 commit into from
Oct 24, 2017
Merged

doc: add recommendations for first timers #16350

merged 1 commit into from
Oct 24, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 20, 2017

Checklist
Affected core subsystem(s)

doc,meta

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 20, 2017
vsemozhetbyt
vsemozhetbyt reviewed Oct 20, 2017
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
pull requests to the Node.js project.
It is recommanded to be extra welcoming to "first-time-contributors",
Copy link
Contributor

Choose a reason for hiding this comment

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

recommanded -> recommended ?

Copy link
Member

Choose a reason for hiding this comment

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

You could also drop the first four words:

Be extra welcoming to "first-time-contributors".

I'd use especially in place of extra and lose the unnecessary quotes and hyphens:

Be especially welcoming to first time contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

COLLABORATOR_GUIDE.md Outdated
@@ -68,6 +70,10 @@ wide-reaching impact), and focused changes that affect only documentation
and/or the test suite, may be landed after a shorter delay if they have
multiple approvals.

For first time contributors, it is recommanded that you ask the author
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. recommanded -> recommended ?
  2. Avoid "you"?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the "it is recommended that you"?:

For first time contributors, ask the author if they have configured...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Trott
Trott reviewed Oct 22, 2017
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
@@ -24,8 +24,10 @@ understand the project governance model as outlined in

## Issues and Pull Requests

Courtesy should always be shown to individuals submitting issues and
Courtesy should **always** be shown to individuals submitting issues and
Copy link
Member

Choose a reason for hiding this comment

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

Not an objection, but just a comment that I try to be very sparing with bold, italics, etc. It gets out of hand pretty easily.

Trott
Trott reviewed Oct 22, 2017
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
@@ -68,6 +70,10 @@ wide-reaching impact), and focused changes that affect only documentation
and/or the test suite, may be landed after a shorter delay if they have
multiple approvals.

For first time contributors, it is recommanded that you ask the author
if they have configured their git username and email to their linking
Copy link
Member

Choose a reason for hiding this comment

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

linking -> liking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Trott
Trott approved these changes Oct 22, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM modulo spelling nits.

jasnell
jasnell reviewed Oct 22, 2017
View reviewed changes
COLLABORATOR_GUIDE.md Outdated
pull requests to the Node.js project.
It is recommanded to be extra welcoming to "first-time-contributors",
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: blank line before this sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

refack
refack commented Oct 23, 2017
View reviewed changes
COLLABORATOR_GUIDE.md
@@ -24,8 +24,9 @@ understand the project governance model as outlined in

## Issues and Pull Requests

Courtesy should always be shown to individuals submitting issues and
pull requests to the Node.js project.
Courtesy should **always** be shown to individuals submitting issues and pull
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not an objection, but just a comment that I try to be very sparing with bold, italics, etc. It gets out of hand pretty easily.

Ack. I felt this is a point worth emphasising both to Collaborators, and to contributors who read this (as on most screens it appears above the fold)

@refack
Copy link
Contributor Author

refack commented Oct 23, 2017

Addressed comment PTAL

@refack
doc: add recommendations for first timers
7671f25 
PR-URL: nodejs#16350
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@refack refack merged commit 7671f25 into nodejs:master Oct 24, 2017
@refack refack deleted the collab-guide-tweaks branch October 24, 2017 14:27
cjihrig pushed a commit that referenced this pull request Oct 25, 2017
@refack @cjihrig
doc: add recommendations for first timers
99fdc1d 
PR-URL: #16350
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@cjihrig cjihrig mentioned this pull request Oct 25, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@refack @addaleax
doc: add recommendations for first timers
1cc05d9 
PR-URL: nodejs/node#16350
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@refack @MylesBorins
doc: add recommendations for first timers
1a3a1b6 
PR-URL: #16350
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@refack @MylesBorins
doc: add recommendations for first timers
44ee353 
PR-URL: #16350
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@refack @MylesBorins
doc: add recommendations for first timers
af869f0 
PR-URL: #16350
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@refack @addaleax
doc: add recommendations for first timers
eb10df6 
PR-URL: nodejs/node#16350
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@jasnell jasnell jasnell left review comments

@gibfahn gibfahn gibfahn approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@Trott Trott Trott approved these changes

@apapirovski apapirovski apapirovski approved these changes

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.
10 participants
@refack @apapirovski @jasnell @Trott @joyeecheung @gireeshpunathil @vsemozhetbyt @gibfahn @MylesBorins @nodejs-github-bot