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

Fixed broken link in onboarding.md. Removed Members team link. #18878

Closed
wants to merge 5 commits into from

Conversation

justin0022
Copy link
Contributor

These two links are broken:

https://github.com/orgs/nodejs/teams/collaborators
https://github.com/orgs/nodejs/teams/members

I've relinked the collaborators to the main README.md's collaborator section, but could not find a member's section to link to, so I removed it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 20, 2018
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.

I can see how this is confusing to a reader who does not have access to the original link. The original link is not broken if you are a member of the nodejs GitHub org with sufficient privileges. Since that is the intended audience of that part of the document, the link should remain as is. The original link is where the interface for adding the individual is, so it is the most useful link. The link to the list of Collaborators in the README, on the other hand, is definitely not where we want the onboarder to add the onboardee because that's the onboardee's exercise later in the doc.

That said, the members team is one where we should probably not be adding people to it, so removing that link is 👍 by me. (I'm slowly-but-surely converting other teams to be sub-teams of the members team, so adding people directly will be unnecessary.)

@justin0022
Copy link
Contributor Author

Thanks for clarifying. I've reverted the link back and left the members link out, as you requested.

* Prior to the onboarding session, add the new Collaborator to
[the Collaborators team](https://github.com/orgs/nodejs/teams/collaborators),
and to [the Members team](https://github.com/orgs/nodejs/teams/members) if
* Prior to the onboarding session, add the new Collaborator to the
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's an extra the at the end of this line (or an extra one at the beginning of the next line).

[the Collaborators team](https://github.com/orgs/nodejs/teams/collaborators),
and to [the Members team](https://github.com/orgs/nodejs/teams/members) if
* Prior to the onboarding session, add the new Collaborator to the
[the Collaborators team](https://github.com/orgs/nodejs/teams/collaborators), if
Copy link
Member

Choose a reason for hiding this comment

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

Totally a nit: but the additional comma isn't necessary. It doesn't hurt anything either. But to reduce the diff to the bare minimum, maybe get rid of it so this line is unchanged?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, this line would be changed either way, the last word comes from the next line on the left side of the diff. Speaking of which, I'd say it should stay on the next line to keep this one under 80 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the comma and got rid of the spare 'the', sorry I missed that!

@@ -15,8 +15,7 @@ onboarding session.
## Fifteen minutes before the onboarding session

* Prior to the onboarding session, add the new Collaborator to
[the Collaborators team](https://github.com/orgs/nodejs/teams/collaborators),
and to [the Members team](https://github.com/orgs/nodejs/teams/members) if
[the Collaborators team](https://github.com/orgs/nodejs/teams/collaborators) if
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't think of this before but they will not already be a part of the nodejs/collaborators team, so the if they are not already part of it can be removed too. :-D

Copy link
Member

Choose a reason for hiding this comment

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

(The if they are not already part of it applied to nodejs/members.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I've made the change now.

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.

Thanks for doing this and sticking with all the little changes. :-D

@justin0022
Copy link
Contributor Author

No problem!

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 21, 2018
@BridgeAR
Copy link
Member

@benjamingr
Copy link
Member

CI failure unrelated, going to land this

@benjamingr
Copy link
Member

Landed in cadc907 - thank you for your contribution!

@benjamingr benjamingr closed this Feb 23, 2018
benjamingr pushed a commit that referenced this pull request Feb 23, 2018
Remove link to the outdated members team

PR-URL: #18878
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
benjamingr pushed a commit that referenced this pull request Feb 23, 2018
Remove link to the outdated members team

PR-URL: #18878
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Feb 26, 2018
Remove link to the outdated members team

PR-URL: nodejs#18878
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 26, 2018
Remove link to the outdated members team

PR-URL: #18878
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@addaleax addaleax mentioned this pull request Feb 27, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Remove link to the outdated members team

PR-URL: nodejs#18878
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants