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

add pull-request etiquette #143

Merged
merged 7 commits into from
Dec 8, 2020
Merged

add pull-request etiquette #143

merged 7 commits into from
Dec 8, 2020

Conversation

escattone
Copy link
Contributor

@escattone escattone commented Dec 3, 2020

@escattone escattone requested a review from a team as a code owner December 3, 2020 01:17
@escattone escattone requested review from a team and removed request for peterbe and chrisdavidmills December 3, 2020 01:17
@escattone escattone closed this Dec 3, 2020
@escattone escattone reopened this Dec 3, 2020
Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Hey @escattone ! Looking mostly good; I've left you a few comments to consider.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

All good stuff! ...except the unusual spelling with the hypen. :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@escattone
Copy link
Contributor Author

@chrisdavidmills I think I've addressed all of your points of feedback. Back to you!

@escattone
Copy link
Contributor Author

Thanks for the review @peterbe! I think I've addressed your concerns as well. Back to you!

Copy link
Contributor

@peterbe peterbe left a comment

Choose a reason for hiding this comment

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

One thing is that people might be confused as to what to put into the pull request description.
If it's solving an open issue just typing "Fixes #1234" is often all you need to say because the issue itself should describe the problem, so the PR implies the solution.

But I can't think of a great thing to suggest there. Perhaps, "If your pull request isn't referring to a specific issue, please try to mention what problem the pull request is solving."

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
escattone and others added 2 commits December 4, 2020 13:23
Co-authored-by: Peter Bengtsson <[email protected]>
Co-authored-by: Peter Bengtsson <[email protected]>
Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

@escattone a few more bits, most of them just nits.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@escattone escattone merged commit 93930e1 into mdn:main Dec 8, 2020
@escattone escattone deleted the initial-pr-policy-1098 branch December 8, 2020 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement policy for reviewing/merging mdn content PR's
3 participants