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 <360251+pfulton@users.noreply.github.com>
Co-authored-by: Rajdeep Chandra <rajrock38@gmail.com>
|
|
||
| ## 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.
There was a problem hiding this comment.
improved the template, does this meet your wants?
| ## 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?
There was a problem hiding this comment.
i can make a follow up ticket for this
| ## 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?
There was a problem hiding this comment.
I like the idea of moving the link to the comment above and removing the checkbox. my goal was to reduce redundancy. its also called out in the Contributing docs.
The code styles are covered by linters but could be convinced otherwise if someone has a different understanding off this list item
|
|
||
| - Google Chrome |
There was a problem hiding this comment.
Do we have this list elsewhere?
There was a problem hiding this comment.
good catch i will make sure thats captured
There was a problem hiding this comment.
this is captured in the README:
spectrum-web-components/README.md
Line 7 in 5950185
Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com>
Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com>
| - `on-hold`: PR is blocked and/or needs more discussion. | ||
| - `Spectrum CSS`: Contains a version bump of Spectrum CSS and will require a review by CSS expert | ||
| - `Component: [Name]`: PR effects this component | ||
|
|
There was a problem hiding this comment.
@castastrophe how does this sound, feel free to modify the language
There was a problem hiding this comment.
This was supposed to be associated with blocked
Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com> Co-authored-by: Nikki Massaro <5090492+nikkimk@users.noreply.github.com>
|
|
||
| ## Types of changes | ||
| Polypane review: |
There was a problem hiding this comment.
Should this be an h3 or something similar to mark it as separate from the steps above (or as a different but related section).
|
|
||
| Do not open up a GitHub issue if the bug is a security vulnerability in Spectrum Web Components, and instead to refer to our [security policy](https://helpx.adobe.com/security/alertus.html). | ||
| ## How can you Contribute |
There was a problem hiding this comment.
How you can contribute reads better to me, but just a nit.
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.