Add PR checklists and code review guidelines

stephaniehobson · stephaniehobson · commit a1e9eb6dd15d · 2026-01-05T15:39:04.000-08:00
diff --git a/docs/code-review/checklists/analytics.md b/docs/code-review/checklists/analytics.md
@@ -0,0 +1,43 @@
+# Analytics Checklist
+
+See [Website Analytics](../../measurement/analytics.md) for full documentation.
+
+## Links `<a href="">`
+
+- [ ] Has a [`data-cta-text` OR `data-link-text`](../../measurement/analytics.md#data-attributes)
+    - [ ] If it has class `mzp-c-button` it is a CTA
+    - [ ] If it has class `mzp-c-cta-link` it is a CTA
+    - [ ] If there are two CTAs with the same value for `data-cta-text` include `data-cta-position`
+    - [ ] Has ONLY ONE of `data-cta-text` or `data-link-text`
+- [ ] If linking to another Mozilla property include `utm` params
+    - [ ] `utm_source` is `www.firefox.com` or `www.mozilla.org`
+    - [ ] `utm_medium` is `referral`
+    - [ ] If the query string is in a variable it is called `params` and the string includes the `?`
+- Download button:
+    - [ ] Use the [appropriate helper](../../firefox-downloads/download-buttons.md#which-helper-should-i-use), don't hardcode these (`download_firefox_thanks`, `google_play_button`, `apple_app_store_button`)
+    - [ ] Include a `download_location` if there are multiple buttons on the page
+
+## Buttons `<button type="button">`
+
+- Download button:
+    - [ ] Download buttons are links for analytics purposes (not buttons)
+- Not a download button:
+    - [ ] [`widget_action` reporting in the dataLayer](../../measurement/analytics.md#widget-action)
+
+## QR Codes
+
+- [ ] Use the [`qrcode` helper](../../development/images.md#qrcode)
+- [ ] Use the app store redirects if applicable and include a product and campaign
+
+## Other
+
+- [ ] New custom events configured in GTM
+- If any of the following are used, check that their custom events will be triggered:
+    - [ ] [Download](../../measurement/analytics.md#product-downloads)
+    - [ ] Mozilla Accounts form
+    - [ ] [Newsletter subscribe](../../measurement/analytics.md#newsletter-subscribe)
+    - [ ] Self-hosted videos
+    - [ ] [Send to Device](../../measurement/analytics.md#send-to-device)
+    - [ ] [Social Share](../../measurement/analytics.md#social-share)
+    - [ ] [VPN subscribe button](../../measurement/analytics.md#begin-checkout)
+    - [ ] [Widget Action](../../measurement/analytics.md#widget-action)
diff --git a/docs/code-review/checklists/css.md b/docs/code-review/checklists/css.md
@@ -0,0 +1,28 @@
+# CSS Checklist
+
+See [Writing CSS](../../development/protocol.md) for full documentation.
+
+## Responsive
+
+- [ ] CSS written mobile-first
+- [ ] In extremely wide viewports, the edges of backgrounds are not visible
+- [ ] In extremely wide viewports, content is restricted to `$max-width`
+- [ ] In narrow viewports, content stacks in a logical reading order
+- [ ] Conditional content displays under correct conditions (logged in, out, Fx, not Fx)
+
+## Best Practices
+
+- [ ] Passes [Stylelint](../../development/coding-style.md)
+- [ ] Components added locally do NOT use `mzp` prefix on classes
+    - [ ] Can still use [other prefix conventions](https://protocol.mozilla.org/docs/contributing/naming)
+- [ ] Prefer classes over element selectors or IDs
+- [ ] Use [mixins](https://protocol.mozilla.org/docs/usage/framework) and [design tokens](https://protocol.mozilla.org/docs/fundamentals/design-tokens) when available
+- [ ] No commented out code
+
+## Localization
+
+- [ ] Test in an RTL language (You may find the [Pseudolocalize addon](https://addons.mozilla.org/en-US/firefox/addon/pseudolocalize/) helpful.)
+- [ ] [BIDI mixin](https://protocol.mozilla.org/docs/usage/framework#bidi) used for any properties which include `left` or `right`
+- [ ] [BIDI mixin](https://protocol.mozilla.org/docs/usage/framework#bidi) used for any values which include `left` or `right`
+- [ ] [BIDI mixin](https://protocol.mozilla.org/docs/usage/framework#bidi) used for any properties which assume LTR (e.g. `background-position`, shorthands like `border-width`)
+
diff --git a/docs/code-review/checklists/downoad-as-default.md b/docs/code-review/checklists/downoad-as-default.md
diff --git a/docs/code-review/checklists/experiments.md b/docs/code-review/checklists/experiments.md
@@ -0,0 +1,26 @@
+# Experiments Checklist
+
+See [A/B Testing](../../measurement/abtest.md) for full documentation.
+
+## General
+
+- [ ] There are [no conflicting experiments](../../measurement/abtest.md#avoiding-experiment-collisions)
+- [ ] Consent is respected
+- [ ] There is a [switch](../../development/feature-switches.md) to disable the experiment
+    - [ ] With the experiment disabled, experiment variations do not load
+    - [ ] With the experiment disabled, experiment code is not loaded
+- [ ] If there are multiple conditions needed to run it, bundle the display logic into one variable
+    - e.g. `is_enabled = switch('switchname') && geo=US && lang.startswith('en')`
+- [ ] Test all variations
+- [ ] Test an unexpected variation
+- [ ] The events which will determine the success of the experiment are [being recorded](../../measurement/abtest.md#recording-the-data)
+    - Usually in GA but sometimes Stub Attribution or FundraiseUp
+    - [ ] If GA: an `experiment_view` event is reported in the DataLayer
+- [ ] If a template was added, it is `noindex` and does not have the canonical or hreflang tags (bug 1442331)
+
+## Traffic Cop Experiments
+
+- [ ] Checks `isApprovedToRun` before activating
+- [ ] Experiment activation logic is sound
+- [ ] Traffic is split between variants as expected
+- [ ] [Cookie support is not necessary or is included](../../measurement/abtest.md#cookies-consent)
diff --git a/docs/code-review/checklists/general.md b/docs/code-review/checklists/general.md
@@ -0,0 +1,24 @@
+# General Checklist
+
+## Manual Testing
+
+- [ ] Tested in Firefox, Safari, and a Chromium browser (see [Browser Support](../../development/browser-support.md))
+- [ ] Tested at these breakpoints:
+    - [ ] 360px
+    - [ ] 768px
+    - [ ] 1024px
+    - [ ] 1280px
+    - [ ] 1366px
+    - [ ] 1440px
+    - [ ] 1536px
+
+## Functionality
+
+- [ ] Code does what it's supposed to do
+- [ ] Code makes logical sense
+- [ ] Code has no unintended consequences
+
+## Maintainability
+
+- [ ] Comments explain non-obvious logic and decisions
+
diff --git a/docs/code-review/checklists/html.md b/docs/code-review/checklists/html.md
@@ -0,0 +1,20 @@
+# HTML Checklist
+
+## Best Practices
+
+- [ ] Only one h1 on the page
+- [ ] Heading levels convey the content hierarchy (with no levels skipped)
+- [ ] Semantic HTML
+- [ ] No validation errors
+- [ ] [Automated a11y tests](../../testing/axe.md) pass
+
+## Links
+
+- [ ] No hardcoded "en-US" in links to Mozilla sites
+- [ ] External links have `rel="external noopener"`
+- [ ] Destination does not 404
+
+## SEO
+
+- [ ] Is there an appropriate page title and description?
+- [ ] Page linked from subnav where applicable
diff --git a/docs/code-review/checklists/index.md b/docs/code-review/checklists/index.md
@@ -0,0 +1,27 @@
+# Code Review Checklists
+
+!!! warning "These checklists are not exhaustive"
+    Look, I can't document all the ways something can go wrong. Just because a PR checks all these boxes doesn't mean it should be approved.
+    
+    Be curious about the code you are reviewing and look for places where it might break or could be improved.
+
+    Think about: functionality, accessibility, analytics, code quality, localization, maintainability (including tests), performance, and security.
+
+
+These checklists are `bedrock` & `springfield` specific and are not intended to be through list of best practices. However, there are some generic best practices included in these checks because they have been commonly overlooked in the past. They should also mostly be checks that have to be done by a human. If we can automate it we should.
+
+- [General](general.md) - Functionality, maintainability, and manual testing
+- [Analytics](analytics.md) - Link tracking, buttons, and custom events
+- [CSS](css.md) - Responsive design, best practices, and localization
+- [Experiments](experiments.md) - A/B testing and Traffic Cop
+- [HTML](html.md) - Best practices, links, and SEO
+- [JavaScript](js.md) - Behavior, error handling, and console output
+- [Localization](l10n.md) - Fluent files, string conventions, and style
+- [Media](media.md) - Images, alt text, and optimization
+- [Tests](tests.md) - Unit and functional tests
+
+Automated checks will also be run by GitHub to check:
+
+- [Pre-commit checks have been run](../../getting-started/install/#pre-commit-hooks) (fluent, ruff, stylelint, etc.)
+- JS unit tests pass
+- Python unit tests pass
diff --git a/docs/code-review/checklists/js.md b/docs/code-review/checklists/js.md
@@ -0,0 +1,13 @@
+# JavaScript Checklist
+
+See [Writing JavaScript](../../development/javascript.md) for conventions on ES5 vs ES6+ syntax.
+
+- [ ] Behaves as expected
+- [ ] Code makes logical sense
+- [ ] Logs [analytics events](../../measurement/analytics.md) as needed
+- [ ] No errors in the console
+- [ ] No `console.log`s
+- [ ] Conditional content displays under correct conditions (mobile vs desktop, Firefox vs other, etc.)
+- [ ] Promise rejections are handled
+- [ ] Has a sensible fallback for when JS is disabled
+
diff --git a/docs/code-review/checklists/l10n.md b/docs/code-review/checklists/l10n.md
@@ -0,0 +1,41 @@
+# Localization Checklist
+
+## Template Verification
+
+- [ ] No hardcoded English strings
+- [ ] All Fluent references are valid
+    - Run `./manage.py l10n_update` to make sure you have the most recent Fluent files
+- [ ] Unused IDs are removed
+
+## String Conventions
+
+- [ ] [Proper organization with subheadings](../../l10n/fluent.md#translating-with-ftl-files)
+- [ ] [Correct brand references](../../l10n/fluent.md#using-brand-names)
+- [ ] [Explanatory comments for idioms and variables](../../l10n/fluent.md#variables)
+- [ ] [Appropriate handling of HTML attributes and link text](../../l10n/fluent.md#html-with-attributes)
+
+## ID Conventions
+
+- [ ] [Lowercase hyphenated naming with filename prefixes](../../l10n/fluent.md#ftl-string-ids)
+- [ ] Version control for altered strings
+- [ ] [Removal notes for fallbacks including date](../../l10n/fluent.md#fallback)
+
+## New File Requirements
+
+- [ ] Fluent files belong in `/l10n/en/`
+- [ ] The top of the file should include the URL of the page on the dev server
+- [ ] [Configured in `pontoon.toml`](../../l10n/configuration.md#specifying_fluent_files)
+- [ ] Connected to templates or views
+
+## Style Standards
+
+- [ ] Lowercase "web" and "internet"
+- [ ] Em dashes have spaces on either side
+- [ ] Sentence case for headings
+- [ ] Use "sign in" / "sign out" (not login/out)
+
+See also:
+
+- [Firefox Style Guide](https://support.mozilla.org/en-US/kb/contributor-style-guide)
+- [Fluent Basics](../../l10n/fluent.md)
+- [Fluent Configuration](../../l10n/configuration.md)
diff --git a/docs/code-review/checklists/media.md b/docs/code-review/checklists/media.md
@@ -0,0 +1,9 @@
+# Media Checklist
+
+See [Working with Images](../../development/images.md) for full documentation on image helpers and optimization.
+
+- [ ] Has alt text where applicable
+- [ ] All images in a `srcset` can be loaded
+- [ ] Use [image helpers](../../development/images.md) (`static()`, `resp_img()`, `picture()`, `l10n_img()`)
+- [ ] Images are optimized
+- [ ] SVG contains viewbox definition
diff --git a/docs/code-review/checklists/tests.md b/docs/code-review/checklists/tests.md
@@ -0,0 +1,14 @@
+# Tests Checklist
+
+See [Testing](../../testing/index.md) for an overview of our testing frameworks.
+
+## Unit Tests
+
+- [ ] Existing [Python (pytest)](../../testing/pytest.md) and [JavaScript (Jasmine)](../../testing/jasmine.md) tests are updated and passing
+- [ ] Any new ones needed?
+
+## Functional Tests
+
+- [ ] Existing [Playwright](../../testing/playwright.md) tests are updated and passing
+- [ ] Any new ones needed?
+- [ ] Do we need to run integration tests before merging this PR?
diff --git a/docs/code-review/index.md b/docs/code-review/index.md
@@ -0,0 +1,81 @@
+# Code Reviews
+
+Code reviews serve two purposes: they help maintain code quality and they're opportunities for knowledge sharing. A good review catches bugs and enforces standards, but it also helps people learn from each other.
+
+This guide focuses on how to be an effective reviewer.
+
+## Response Time Expectations
+
+Review timing depends on the priority of work and the time zone of the reviewer:
+
+| Type | Expected Response |
+|------|-------------------|
+| Security fixes / bug fixes on a deadline | Same day (please ping a reviewer on Slack) |
+| Re-review on requested changes. | Same or next day. Please keep the review cycle short once it’s begun. |
+| On roadmap / planned work | 1-2 business days. (Please share a link in your status update) |
+| Contributor / unplanned work | When the team has capacity |
+| Drafts | Will not be reviewed unless you specifically ask for feedback |
+
+If you're assigned as a reviewer but can't review in time, communicate that so the author can find another reviewer.
+
+## What to Review
+
+Reviewers should be thinking about: functionality, accessibility, analytics, code quality, localization, maintainability (including tests), performance, and security.
+
+We have some [code review checklists](checklists/index.md) to help avoid common problems.
+
+## Giving Feedback
+
+### Tone and Approach
+
+- **Be constructive and kind.** Remember there's a person on the other end.
+- **Focus on the code, not the person.** Say "This function could be simplified" rather than "You wrote this in a confusing way."
+- **Acknowledge good solutions.** It's easy to only comment on problems. When you see something clever or well-done, say so.
+- **Assume good intent.** If something looks wrong, ask about it before assuming it's a mistake.
+- [**Aim to bring the code up a letter grade or two.**](https://mtlynch.io/human-code-reviews-2/#aim-to-bring-the-code-up-a-letter-grade-or-two) When there are many nitpicks or optional changes, limit what you're asking the author to change. 
+
+### Feedback Format
+
+Indicate clearly if feedback is blocking.
+
+Some team members use [Conventional Comments](https://conventionalcomments.org/) but this is not required.
+
+Examples of conventional comments:
+
+- `praise: Clever use of grids!`
+- `nitpick: Could use a more descriptive variable name here, but not a big deal.`
+- `question: Should this page be excluded from search indexing?`
+- `issue (blocking): This will break in production because X.`
+- `chore (blocking): Please update this to use the existing helper in utils.py.`
+
+### Style Disagreements
+
+If there is a disagreement about code style or approach:
+
+- Mozilla Staff front/back end leads get final say on critical decisions.
+    - If a lead has to step in and make a decision, we should update the documentation to avoid the discussion in the future.
+- Documentation, style guides, and config must be adhered to.
+- PR author gets to make non-critical decisions.
+
+## Approving and Merging
+
+- **Approve and merge:** If all feedback is addressed or there is no feedback.
+- **Approve but leave open:** If there are minor changes you trust the author to address.
+- **Request changes:** Only for issues that must be fixed before merging.
+- **Comment:** If your feedback is non blocking or you have not completed your code review.
+
+If you approved with comments, consider adding a note like "Approved with minor suggestions - feel free to merge after addressing or ignore if you disagree."
+
+You may also see these abbreviations on our repos: 
+
+- **R+** = Approved.
+- **R+wc** = Approved with requested changes.
+- **R+ emoji** = Approved with a fun emoji. Don't think too hard about it, or have fun trying to figure out why this particular emoji was picked. For example: you might see R+🧼 (soap) for a PR which does code clean up, R+🫖 (tea pot) for a typo fix in en-GB or R+🧛(vampire) for a PR on VPN because it came up when the reviewer searched for emojis beginning with "V".
+
+## Resources
+
+- [How to Make Your Code Reviewer Fall in Love with You](https://mtlynch.io/code-review-love/)
+- [How to Do Code Reviews Like a Human](https://mtlynch.io/human-code-reviews-1/)
+- [How to Do Code Reviews Like a Human (Part Two)](https://mtlynch.io/human-code-reviews-2/)
+- [Code Review Anxiety Workbook](https://developer-success-lab.gitbook.io/code-review-anxiety-workbook-1)
+- [Conventional Comments](https://conventionalcomments.org/)
diff --git a/docs/css/project-tabs.css b/docs/css/project-tabs.css
@@ -111,3 +111,18 @@ label[data-project="springfield"]:hover,
     -webkit-mask-image: var(--md-admonition-icon--both);
     mask-image: var(--md-admonition-icon--both);
 }
+
+/**
+ * Task list checkboxes - focus styles
+ */
+.task-list-control input[type="checkbox"]:focus-visible + .task-list-indicator:after {     
+    content: '';
+    display: block;
+    height: 1.25em;
+    left: -1.5em;
+    outline-offset: 2px;
+    outline: 2px solid var(--md-accent-fg-color);
+    position: absolute;
+    top: .15em;
+    width: 1.25em;
+}
diff --git a/mkdocs.yml b/mkdocs.yml
@@ -86,6 +86,9 @@ markdown_extensions:
       slugify: !!python/object/apply:pymdownx.slugs.slugify
         kwds:
           case: lower
+  - pymdownx.tasklist:
+      custom_checkbox: true
+      clickable_checkbox: true
   - pymdownx.tilde
 
 extra_javascript:
@@ -126,6 +129,20 @@ nav:
     - Axe (Accessibility): testing/axe.md
     - Selenium (Legacy): testing/selenium.md
 
+  - Code Review:
+    - Code Review: code-review/index.md
+    - Checklists: 
+      - Checklists: code-review/checklists/index.md
+      - General: code-review/checklists/general.md
+      - Analytics: code-review/checklists/analytics.md
+      - CSS: code-review/checklists/css.md
+      - Experiments: code-review/checklists/experiments.md
+      - HTML: code-review/checklists/html.md
+      - JavaScript: code-review/checklists/js.md
+      - Localization: code-review/checklists/l10n.md
+      - Media: code-review/checklists/media.md
+      - Tests: code-review/checklists/tests.md
+
   - Localization:
     - Localization: l10n/index.md
     - Fluent Basics: l10n/fluent.md
