docs: improve contributing and pull request documentation #5387
Conversation
|
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
| 1. Check the Open Issues to see if the problem has already been reported. | ||
| 1. TIP: Apply the component label to make your search process more straightforward. | ||
| 2. If it has and the issue is still open, add a comment to the existing issue instead of opening a new one. | ||
| 3. Check if you can reproduce the problem in the latest version of Spectrum Web Components. |
There was a problem hiding this comment.
we should append this with our new isolation steps in another section once we land on a new tool
|
|
||
| We support all viewport sizes across supported desktop browsers. | ||
| Avoid editing distribution files (if present). Make changes to the source files, then allow the build system to generate any bundled or output files automatically. |
There was a problem hiding this comment.
we can update this line after cutoff
Co-authored-by: Patrick Fulton <[email protected]>
Co-authored-by: Rajdeep Chandra <[email protected]>
|
|
||
| ## How has this been tested? | ||
|
|
||
| <!--- Please describe in detail how you tested your changes. --> | ||
| <!--- Include details of your testing environment, and the tests you ran to see how your change affects other areas of the code, etc. --> | ||
|
|
||
| - [ ] _Test case 1_ | ||
| - [ ] _Descriptive Test Statement_ |
There was a problem hiding this comment.
Would it make sense to give a practical example here to help contributors? For example, Open [link]() and click the submit button. Expect the state of the button to change to active.
| @@ -1,4 +1,5 @@ | |||
| <!--- Provide a general summary of your changes in the Title above --> | |||
| <!--- PR titles should follow conventional commit and should include commit type as defined in in the PULL_REQUESTS guide --> | |||
There was a problem hiding this comment.
Should we link to the docs to help folks unfamiliar with conventional commits? https://www.conventionalcommits.org/en/v1.0.0/#summary
| ## Motivation and context | ||
|
|
||
| <!--- Why is this change required? What problem does it solve? --> | ||
| - fixes [Issue Number] | ||
|
|
||
| ## How has this been tested? | ||
|
|
||
| <!--- Please describe in detail how you tested your changes. --> |
There was a problem hiding this comment.
Maybe a nice to-do would be to link to a doc outlining how to write test cases and requirements for PR validation?
| - [ ] I have reviewed at the Accessibility Practices for this feature, see: [Aria Practices](https://www.w3.org/TR/wai-aria-practices/) | ||
| - [ ] I have added tests to cover my changes. | ||
| - [ ] I have ensured documentation covers my changes. |
There was a problem hiding this comment.
| - [ ] I have ensured documentation covers my changes. | |
| - [ ] If my change required an update to the documentation, I have included it in this pull request. |
I quite like the original wording personally - it's a little more explicit in requiring those docs updates be in this PR.
| ## Checklist | ||
|
|
||
| <!--- Go over all the following points, and put an `x` in all the boxes that apply. If you're unsure about any of these, don't hesitate to ask. We're here to help! --> | ||
|
|
||
| - [ ] I have signed the [Adobe Open Source CLA](http://opensource.adobe.com/cla.html). | ||
| - [ ] My code follows the code style of this project. |
There was a problem hiding this comment.
Is the idea of removing this here because we employ linters?
There was a problem hiding this comment.
Ooh, I also think we should keep these checks.
There was a problem hiding this comment.
I do love the idea of removing the CLA checkbox because the PR requires it programmatically anyway. Maybe just a link instead to the resource if someone needs to go sign it?
| #### For Reviewers | ||
|
|
||
| - **Be Specific and Constructive**: Provide clear, actionable feedback rather than vague criticisms. Explain why a change is needed. | ||
| - **Prioritize Issues**: Focus on important issues first (architecture, functionality, performance) before minor stylistic concerns. |
There was a problem hiding this comment.
How can we reword some of these to focus on the positive outcomes instead of the negative past that might have led to these guidelines being formed?
| - **Be Specific and Constructive**: Provide clear, actionable feedback rather than vague criticisms. Explain why a change is needed. | ||
| - **Prioritize Issues**: Focus on important issues first (architecture, functionality, performance) before minor stylistic concerns. | ||
| - **Ask Questions**: When something isn't clear, ask questions rather than making assumptions about intent. | ||
| - **Suggest Alternatives**: When pointing out issues, suggest possible solutions or approaches. Leverage the code suggestions feature to streamline applying feedback for the author. |
There was a problem hiding this comment.
Can we connect this to the be specific and constructive heading? They seem intrinsically related.
| - **Prioritize Issues**: Focus on important issues first (architecture, functionality, performance) before minor stylistic concerns. | ||
| - **Ask Questions**: When something isn't clear, ask questions rather than making assumptions about intent. | ||
| - **Suggest Alternatives**: When pointing out issues, suggest possible solutions or approaches. Leverage the code suggestions feature to streamline applying feedback for the author. | ||
| - **Recognize Good Work**: Acknowledge well-written code and good design decisions. Positive reinforcement is valuable. |
There was a problem hiding this comment.
| - **Recognize Good Work**: Acknowledge well-written code and good design decisions. Positive reinforcement is valuable. | |
| - **Celebrate good work**: By acknowledging well-written code and good design decisions, we celebrate our contributors and the hard work they put in. Positive reinforcement is valued and makes for a fun and engaging project! |
| - **Ask Questions**: When something isn't clear, ask questions rather than making assumptions about intent. | ||
| - **Suggest Alternatives**: When pointing out issues, suggest possible solutions or approaches. Leverage the code suggestions feature to streamline applying feedback for the author. | ||
| - **Recognize Good Work**: Acknowledge well-written code and good design decisions. Positive reinforcement is valuable. | ||
| - **Remember Context**: Consider the PR author's experience level and the scope of changes when providing feedback. |
There was a problem hiding this comment.
I like this generally but it's kind of an unknowable factor in code reviews with non-team contributors. What does this advice look like in practice?
| - **Recognize Good Work**: Acknowledge well-written code and good design decisions. Positive reinforcement is valuable. | ||
| - **Remember Context**: Consider the PR author's experience level and the scope of changes when providing feedback. | ||
| - **Be Timely**: Complete reviews promptly to avoid blocking progress. | ||
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote its non-blocking feedback. |
There was a problem hiding this comment.
How can we reword this to focus on the positive outcome we're wanting?
There was a problem hiding this comment.
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote its non-blocking feedback. | |
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote it's non-blocking feedback. |
nit: apostrophe 😉
There was a problem hiding this comment.
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote its non-blocking feedback. | |
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote that it is non-blocking feedback. |
| ## Motivation and context | ||
|
|
||
| <!--- Why is this change required? What problem does it solve? --> | ||
| - fixes [Issue Number] | ||
|
|
||
| ## How has this been tested? | ||
|
|
||
| <!--- Please describe in detail how you tested your changes. --> |
There was a problem hiding this comment.
Can we also indicate this section should include steps is to help reviewers test as well.
|
|
||
| ### Contributor License Agreement | ||
| If you work for Adobe, our slack channel #spectrum_web_components has some great workflows to get you started. Be sure to read the Welcome Canvas when you join. |
There was a problem hiding this comment.
| If you work for Adobe, our slack channel #spectrum_web_components has some great workflows to get you started. Be sure to read the Welcome Canvas when you join. | |
| If you work for Adobe, our Slack channel #spectrum_web_components has some great workflows to get you started. Be sure to read the Welcome Canvas when you join. |
|
|
||
| - Suggest your change in the [ideas list](https://github.com/adobe/spectrum-web-components/discussions/categories/ideas) and start writing code. | ||
| 1. Check the Open Issues to see if the problem has already been reported. |
There was a problem hiding this comment.
| 1. Check the Open Issues to see if the problem has already been reported. | |
| 1. Check the [Open Issues](https://github.com/adobe/spectrum-web-components/issues) to see if the problem has already been reported. |
| - TIP: Apply the component label to make your search process more straightforward. | ||
| 2. If it has and the issue is still open, add a comment to the existing issue instead of opening a new one. | ||
| 3. Check if you can reproduce the problem in the latest version of Spectrum Web Components. | ||
| 4. If there are no related open issues and it is reproducible in isolation, then open a Bug Report. |
There was a problem hiding this comment.
| 4. If there are no related open issues and it is reproducible in isolation, then open a Bug Report. | |
| 4. If there are no related open issues and it is reproducible in isolation, then open a [Bug Report](https://github.com/adobe/spectrum-web-components/issues/new?template=bug_report.yaml). |
|
|
||
| - Google Chrome |
There was a problem hiding this comment.
Do we have this list elsewhere?
|
|
||
| - Description of the changes | ||
| - Related issues (using proper [GitHub keywords](https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue) to auto-close issues i.e. `fixes`, `resolves`, or `closes`) | ||
| - Type of change in the PR title (bug fix, feature, breaking change) |
There was a problem hiding this comment.
| - Type of change in the PR title (bug fix, feature, breaking change) | |
| - Type of change in the PR title (bug fix, feature, breaking change) | |
| - Steps you took to test your changes that reviewers can follow to also test them |
| - **Recognize Good Work**: Acknowledge well-written code and good design decisions. Positive reinforcement is valuable. | ||
| - **Remember Context**: Consider the PR author's experience level and the scope of changes when providing feedback. | ||
| - **Be Timely**: Complete reviews promptly to avoid blocking progress. | ||
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote its non-blocking feedback. |
There was a problem hiding this comment.
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote its non-blocking feedback. | |
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote that it is non-blocking feedback. |
| - **Remember Context**: Consider the PR author's experience level and the scope of changes when providing feedback. | ||
| - **Be Timely**: Complete reviews promptly to avoid blocking progress. | ||
| - **Avoid Nitpicking**: Focus on meaningful improvements rather than personal style preferences that don't violate project standards. Prepend comments with `nit:` to denote its non-blocking feedback. | ||
| - **Use Changes Requested Sparingly**: Changes Requested review status should only be used in instances where a bug or regression is still present in the PR. |
There was a problem hiding this comment.
Maybe move this one above nit-picking
|
|
||
| - README contains a clear description and minimal example | ||
| - Inline documentation for all public APIs | ||
| - Accessibility documentation that aligns with WCAG patterns |
There was a problem hiding this comment.
| - Accessibility documentation that aligns with WCAG patterns | |
| - Accessibility documentation that aligns with WCAG patterns | |
| See [Documenting a component](https://opensource.adobe.com/spectrum-web-components/guides/adding-component/#documenting-the-component) for more information on our documentation standards and structure. |
| - Inline documentation for all public APIs | ||
| - Accessibility documentation that aligns with WCAG patterns | ||
|
|
||
| #### API Documentation |
There was a problem hiding this comment.
Should we refer back to JSDoc here?
Description
This PR introduces:
The goal of this PR is to be COLLABORATIVE. Nothing is set in stone and is absolutely up for discussion and amendment. All reviewers are encouraged to add comments and questions to have discussion.
Related issue(s)
How has this been tested?
Read Contributing doc
Read Pull requests doc
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.