Skip to content

Commit 34fbcad

Browse files
docs: update pr review guidance (#6154)
* docs: first pass of pr review notes summary * docs: integrate pr notes into existing pr reviews docs
1 parent e0b0ec2 commit 34fbcad

1 file changed

Lines changed: 70 additions & 29 deletions

File tree

CONTRIBUTOR-DOCS/01_contributor-guides/05_participating-in-pr-reviews.md

Lines changed: 70 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
- [Review timing](#review-timing)
1717
- [Review expectations](#review-expectations)
1818
- [Review etiquette](#review-etiquette)
19+
- [Scope, workflow, and status](#scope-workflow-and-status)
20+
- [References and alignment](#references-and-alignment)
21+
- [Reviewer quick reference](#reviewer-quick-reference)
1922
- [Merge criteria](#merge-criteria)
2023

2124
</details>
@@ -26,13 +29,13 @@ This document outlines our team's expectations and best practices for reviewing
2629

2730
## Labels and their meanings
2831

29-
[See the complete list of labels on Github. ](https://github.com/adobe/spectrum-web-components/labels)
32+
[See the complete list of labels on GitHub.](https://github.com/adobe/spectrum-web-components/labels)
3033

3134
- `Contribution`: This label denotes the PR is from someone other than the maintainers of SWC.
3235
- `Status: Ready for review`: PR is ready for maintainer review
33-
- `Status: Ready for merge`: PR has two approvals and all tests pass.
36+
- `Status: Ready for merge`: PR has two approvals and all tests pass.
3437
- `Status: WIP`: PR is still being worked on, not ready for review
35-
- `Status: Blocked`: PR is blocked for some reason, eg another PR needs to go in first
38+
- `Status: Blocked`: PR is blocked for another reason, for example another PR needs to merge first
3639
- `Status: On hold`: PR on hold pending more discussion.
3740
- `Status: Addressing feedback`: PR owner is addressing review comments and will request re-review when ready.
3841
- `Status: Ready for design review`: PR needs to be checked by the Spectrum Design team
@@ -48,8 +51,10 @@ Apply labels promptly to help maintainers prioritize and manage the review queue
4851

4952
### Review timing
5053

51-
- Maintainers aim to review PRs in a timely manner
52-
- If your PR hasn't received attention, feel free to ping the team in the PR comments
54+
- Maintainers aim to review PRs in a timely manner.
55+
- When you can, aim to **respond within about a business day**. Treat **blockers** and **high-priority** PRs (see `High priority PR review`) as **same-day** or faster when feasible.
56+
- If your PR has not received attention, feel free to **ping the team** in the PR comments.
57+
- Agree as a team how **hot fixes**, **expected review duration**, and **re-review** handoffs work, including when to **add another reviewer** if someone is unavailable.
5358

5459
### Review expectations
5560

@@ -58,9 +63,20 @@ Reviewers will check for:
5863
- Adherence to code style and component patterns
5964
- Proper test coverage
6065
- Documentation completeness
61-
- **Accessibility compliance:** The PR author has completed the [Accessibility testing checklist](https://github.com/adobe/spectrum-web-components/blob/main/.github/PULL_REQUEST_TEMPLATE.md#accessibility-testing-checklist) in the pull request template. For PRs that affect interactive components, verify that **keyboard testing steps** and **screen reader testing steps** are documented (where tested, what was done, expected result). When in doubt, re-run key steps to confirm behavior. See [Accessibility testing — Manual testing required for PRs](09_accessibility-testing.md#manual-testing-required-for-prs) for guidelines.
62-
- Visual regression test coverage
66+
- **Accessibility compliance:** The PR author has completed the [Accessibility testing checklist](https://github.com/adobe/spectrum-web-components/blob/main/.github/PULL_REQUEST_TEMPLATE.md#accessibility-testing-checklist) in the pull request template. For PRs that affect interactive components, verify that **keyboard testing steps** and **screen reader testing steps** are documented (where tested, what was done, expected result). When in doubt, re-run key steps to confirm behavior. See [Accessibility testing — Manual testing required for PRs](09_accessibility-testing.md#manual-testing-required-for-prs) for guidelines. Do not ship **regressions** in accessibility, semantics, or documented behavior.
67+
- Visual regression test coverage; use **Chromatic** with a **healthy baseline** so diffs stay trustworthy (see [Visual regression testing](../02_style-guide/04_testing/04_visual-regresssion-testing.md)). For a concise reviewer checklist, see [PR review checklist](../02_style-guide/04_testing/09_pr_review-checklist.md).
6368
- Performance considerations
69+
- **Depth beyond the template:** Treat the author's validation steps as a **minimum**. Exercise **unhappy paths** and **error or empty states** in good faith. **Check out the branch locally** when it helps; validate **dependencies** and behavior in contexts closer to **production**, not only Storybook.
70+
- **Layout and reflow:** Where relevant, consider **responsive** behavior, realistic copy, and **WCAG** concerns such as zoom and reflow.
71+
- **Integration:** Consider how the change behaves with **surrounding UI**, APIs, and other components, not only the edited file.
72+
- **Implicit checks:** When you use **Windows High Contrast Mode**, combined size/variant/state setups, or other checks that are easy to omit, **note them in a comment** so authors and future reviewers learn the bar.
73+
- **User-centric validation:** When behavior is easy to misread, authors should add steps in the PR description (for example **Given/When/Then** or a short **user scenario**); reviewers should run them.
74+
75+
#### Storybook, Chromatic, and playground demos
76+
77+
- Prefer **Playground** stories when you need to **probe** unclear behavior, **edge cases**, or **atomic** components in isolation.
78+
- Decide deliberately whether a scenario belongs in **Storybook**, in **Docs** only, or in a **StackBlitz-style** demo; favor Storybook when the example should stay **maintained** as part of the design system.
79+
- **Flex and grid wrappers** in Storybook can **stretch or shrink** small components; verify critical layouts in a **neutral** container when sizing matters.
6480

6581
### Review etiquette
6682

@@ -70,37 +86,62 @@ Both reviewers and PR authors should follow these guidelines:
7086

7187
#### For reviewers
7288

73-
- **Maintain momentum**: Complete reviews in a timely manner to keep the project moving forward.
74-
- **Provide clear, actionable feedback**: Help contributors succeed by offering specific guidance and explaining the reasoning behind suggested changes.
75-
- **Offer solutions**: When identifying areas for improvement, suggest alternative approaches and use code suggestions to make implementation easier.
76-
- **Seek understanding**: Ask questions to clarify intent and approach, fostering a collaborative environment for learning and improvement.
77-
- **Recognize excellence**: Celebrate well-written code and thoughtful design decisions to encourage continued high-quality contributions.
78-
- **Focus on impact**: Prioritize feedback on architecture, functionality, and performance to ensure the most important aspects are addressed first.
79-
- **Focus on value**: Prioritize feedback that improves code quality and maintainability over personal style preferences. If you make a suggestion that is non-blocking feedback, prepend the comment with `nit:`.
80-
- **Consider context**: Tailor feedback to the PR author's experience level and the scope of changes.
81-
- **Use changes requested thoughtfully**: Reserve the Changes Requested status for instances where critical issues need to be addressed.
82-
- **Review VRTs with care**: Thoroughly examine visual regression test results and communicate approval status to authors.
89+
- **Maintain momentum** and complete reviews in a timely manner (see [Review timing](#review-timing)).
90+
- **Provide clear, actionable feedback** with **brief rationale**; use **code suggestions** when they speed up implementation.
91+
- **Seek understanding:** Ask questions; default to **curiosity** over directives when you are exploring intent. If you are unsure of the right fix, **say so** or **look it up briefly** instead of leaving the author guessing.
92+
- **Stay constructive:** Keep comments **clear, kind, and informative**; **name what works**, not only what to change. **Call out strong choices** so good habits spread.
93+
- **Prefer patterns over nits:** Focus on themes that improve quality; if feedback is **non-blocking**, prepend the comment with `nit:` (see also [Conventional Comments](https://conventionalcomments.org/) for labeling blocking vs questions vs suggestions vs praise).
94+
- **Link to standards:** Point to **docs, prior art, or ADRs** when that shortens the loop.
95+
- **Focus on impact and value:** Prioritize architecture, functionality, accessibility, and performance; **consider context** such as the author's experience level and PR scope.
96+
- **Use Changes Requested thoughtfully:** Reserve it for **blocking** or **high-risk** issues; use inline comments and suggestions for preferences and nits when appropriate.
97+
- **Review VRTs with care:** Examine visual regression results and communicate approval to authors before golden images are updated.
98+
- **Collaboration modalities:** Use **short video or live walkthroughs** when the UI is **complex** or hard to show in screenshots; afterward, **post a summary on the PR** so decisions stay discoverable. For **high-impact** changes that are easy to misread in the diff, a **brief sync** or **PR kickoff** can save rounds of review. **DM or huddle** is fine for thrashing out an approach; **capture the outcome on the PR**.
99+
- **Multiple perspectives:** Welcome a second view; optionally read the diff **before** other reviewers' comments for an **independent** take. Align as a team on **second reviews** after a subject-matter expert approves, behavior when someone **requests changes**, and **who updates** GitHub and tracker labels.
100+
- **Automation:** Consider **linting and CI** (and, where helpful, other tooling) for a first pass on formatting and obvious issues so human review can focus on design and correctness.
83101

84102
#### For PR authors
85103

86-
- **Self review your PR**: Take a first pass at reviewing and commenting on your own submission. This gives reviewers valuable context and may pre-emptively answer questions that might otherwise arise.
87-
- **Resolve/respond to all comments**: Address each review comment, either with code changes or explanations of your approach.
88-
- **Ask for clarification**: If review feedback is unclear, ask questions to understand the concern.
89-
- **Notify when ready**: After addressing feedback, notify reviewers that the PR is ready for another look either in Slack or by requesting a new review in GitHub.
90-
- **Explain complex changes**: For non-obvious changes, explain your reasoning in the PR description or comments.
91-
- **Break down large PRs**: When possible, split large changes into smaller, more manageable PRs.
92-
- **Test thoroughly**: Before requesting review, ensure your code meets the project's quality standards.
104+
- **Self-review:** Run your own **validation** and review the diff before requesting reviewers; a first pass in comments can pre-empt questions.
105+
- **Explain and curate:** For non-obvious changes, explain **why** in the description. **Curate commits** (for example with **interactive rebase**) so history tells a clear story—see guides such as *Advanced techniques in Git: interactive rebasing* and *Curating commits to speed up pull requests* for technique.
106+
- **Track deferred work:** Use **tickets** for follow-ups; **TODOs in code** should reference a **tracked issue** (for example Jira). Involve **design** when the spec or edge cases are unclear.
107+
- **Help reviewers validate:** Add **reviewer-friendly steps** (for example **Gherkin-style** or a short **user scenario**) when behavior is easy to misread.
108+
- **Respond and notify:** Resolve or respond to **every** comment; ask for clarification when feedback is unclear; after addressing feedback, **notify** reviewers (Slack or **request re-review** on GitHub).
109+
- **Re-read the whole PR** while addressing feedback so fixes do not introduce regressions elsewhere.
110+
- **Treat review as a dialogue:** You do not need to accept every suggestion; **defer** or open a **follow-up PR** when that is cleaner.
111+
- **Keep status visible:** Use **labels**, **Jira**, **dashboards**, and **Slack threads** on the original announcement as your squad expects; make **priority** obvious for urgent work.
112+
- **Scope:** Prefer **small, focused PRs**; see [Scope, workflow, and status](#scope-workflow-and-status).
93113

94114
#### Resolving disagreements
95115

96-
- **Focus on data**: Back up opinions with data, documentation, or examples where possible.
97-
- **Refer to standards**: Use project conventions and industry best practices to guide decisions.
98-
- **Compromise when appropriate**: Be willing to find middle ground when opinions differ.
99-
- **Escalate respectfully**: If consensus can't be reached, involve a third team member or technical lead for guidance.
100-
- **Document decisions**: Record the reasoning behind significant technical decisions for future reference.
116+
- **Focus on data:** Back up opinions with data, documentation, or examples where possible.
117+
- **Refer to standards:** Use project conventions and industry best practices to guide decisions.
118+
- **Compromise when appropriate:** Be willing to find middle ground when opinions differ.
119+
- **Escalate respectfully:** If consensus cannot be reached, involve a third team member or technical lead for guidance.
120+
- **Document decisions:** Record the reasoning behind significant technical decisions on the PR; treat the thread as a **decision record**, not only a code handoff.
101121

102122
Remember that code reviews are a collaborative process aimed at improving code quality, knowledge sharing, and maintaining project standards. Approaching reviews with empathy and professionalism benefits everyone involved.
103123

124+
### Scope, workflow, and status
125+
126+
- **Small, focused PRs** reduce review fatigue; plan scope **before** opening the PR when possible so reviewers are not asked to split work late.
127+
- Strong candidates for a **split**: unrelated features or fixes in one PR; mixing **refactor, feature, and bugfix**; or a **behavior change** bundled with a large **structural** refactor. If a reviewer cannot grasp **what changed and why** in a few minutes, improve the **description**, **commits**, or **scope**.
128+
- Decide at **refinement or ticket breakdown** whether work ships as **one PR or several**.
129+
- When you find **bugs outside the ticket**, use a team default: **follow-up ticket**, **new branch**, or a **narrow in-scope fix**—and track it.
130+
- **Open a new branch** to **prevent scope creep** when needed; branching is cheap.
131+
- Only skip **docs, tests, or accessibility** updates when the team has an **explicit** agreement (for example **feature branch** work under an **epic** with **tracked follow-ups**).
132+
133+
### References and alignment
134+
135+
- **[Conventional Comments](https://conventionalcomments.org/)** can clarify what is blocking versus optional.
136+
- **[Working agreements](https://www.swarms.com/blog/agile-team-working-agreements/)** are one way to capture shared norms (review load, draft vs ready-for-review and tracker states, **AI** assistance in review or drafting, **external contributor** expectations and stale branches, and similar topics).
137+
138+
### Reviewer quick reference
139+
140+
1. Exercise **unhappy paths**, not only the happy path.
141+
2. **Prioritize** urgent and **blocking** PRs.
142+
3. Validate **beyond** the written steps; check **integration** and realistic contexts.
143+
4. Use a **new branch** instead of letting **scope creep** onto one PR.
144+
104145
---
105146

106147
## Merge criteria

0 commit comments

Comments
 (0)