Add code review checklists

docs/docs/attribution/0001-analytics.md

@@ -65,18 +65,11 @@ CTA ("Call to Action") click is intented to track the one or two main actions th
 
 The attribute `data-cta-text` must be present to trigger the event. All links to accounts.mozilla.com must also use `data-cta-type`.
 
-| Data Attribute      | Expected Value                                                                                                                                                                                                                     |
-| ------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `data-cta-text` *  | Text or name of the link (e.g. `Sign Up`, `Start Here`, `Get Relay`, `See your report`, `Read the Manifesto`).                                                                                                                     |
-|                     |                                                                                                                                                                                                                                    |
-|                     | -   This does not need to exactly match the text displayed to the user                                                                                                                                                             |
-|                     | -   Defining this is useful to group the link clicks across locales                                                                                                                                                                |
-|                     | -   -   this attribute is required                                                                                                                                                                                                 |
-| `data-cta-position` | Location of CTA on the page (e.g. `primary`, `secondary`, `banner`, `pricing`)                                                                                                                                                     |
-| `data-cta-type`     | fxa-servicename (e.g. `fxa-sync`, `fxa-monitor`, `fxa-vpn`, `monitor`, `relay`)                                                                                                                                          |
-|                     |                                                                                                                                                                                                                                    |
-|                     | -   This is to group CTAs by their destination                                                                                                                                                                                     |
-|                     | -   Do not use this to identify the element (ie. link, button)                                                                                                                                                                     |
+| Data Attribute | Expected Value |
+| -------------- | -------------- |
+| `data-cta-text` *  | Text or name of the link (e.g. `Sign Up`, `Start Here`, `Get Relay`, `See your report`, `Read the Manifesto`). <br>- This does not need to exactly match the text displayed to the user<br>-   Defining this is useful to group the link clicks across locales <br>* This attribute is required |
+| `data-cta-position` | Location of CTA on the page (e.g. `primary`, `secondary`, `banner`, `pricing`) |
+| `data-cta-type`     | fxa-servicename (e.g. `fxa-sync`, `fxa-monitor`, `fxa-vpn`, `monitor`, `relay`) <br>- This is to group CTAs by their destination  <br>- Do not use this to identify the element (ie. link, button) |
 | `data-cta-name`     | A identifier for this cta that is unique across the entire site. (e.g. `fx20-primarycta`, `wnp118-sfaq-so-special-features`). This is to help with reporting since it is difficult to filter on more than one parameter at a time. |
 
 ``` html
@@ -93,10 +86,10 @@ Link click is intended to track links that are of interest but not the focus of
 
 The attribute `data-link-text` must be present to trigger the event.
 
-| Data Attribute       | Expected Value                                                                                                               |
-| -------------------- | ---------------------------------------------------------------------------------------------------------------------------- |
+| Data Attribute | Expected Value |
+| -------------- | -------------- |
 | `data-link-text` *  | Text or name of the link (e.g. `Monitor`, `Features`, `Instagram (mozilla)`, `Mozilla VPN`). - * this attribute is required |
-| `data-link-position` | Location of CTA on the page (e.g. `topnav`, `subnav`, `body`, `features`)                                                    |
+| `data-link-position` | Location of CTA on the page (e.g. `topnav`, `subnav`, `body`, `features`) |
 
 ``` html
 <p>This is text with a <a href="#" data-link-text="simple">simple</a>example.</p>
@@ -292,46 +285,40 @@ TrackProductDownload.sendEvent(customEventObject);
 
 We are using the custom event `widget_action` to track the behaviour of javascript widgets.
 
-**How do you chose between \`\`widget_action\`\` and \`\`cta_click\`\`?**
-
-| widget_action                                               | cta_click                                                |
-| ----------------------------------------------------------- | -------------------------------------------------------- |
-| The action is specific or unique.                           | The action is \"click\".                                 |
-|                                                             |                                                          |
-| *(Only the language switcher changes* *the page language.)* |                                                          |
-| The user does not leave the page.                           | It sends the user somewhere else.                        |
-| It requires Javascript to work.                             | No JS required.                                          |
-| It can perform several actions.                             | It does one action.                                      |
-|                                                             |                                                          |
-| *(A modal can be opened and closed.)*                       |                                                          |
-| There could be several on the page doing different things.  | There could be several on the page doing the same thing. |
-|                                                             |                                                          |
-| *(An accordion list of FAQs)*                               | *(A download button in the header and footer.)*          |
+**How do you chose between `widget_action` and `cta_click`?**
+
+| widget_action | cta_click |
+| ------------- | --------- |
+| The action is specific or unique.<br>*(example: only the language switcher changes the page language.)* | The action is "click". |
+| The user does not leave the page. | It sends the user somewhere else. |
+| It requires Javascript to work. | No JS required. |
+| It can perform several actions. <br>*(example: modal can be opened and closed.)*  | It does one action.|
+| There could be several on the page doing different things. <br>*(An accordion list of FAQs)*  | There could be several on the page doing the same thing. <br> *(A download button in the header and footer.)*|
 
 Properties for use with ``widget_action`` (not all widgets will use all options):
 
 - type
     - **Required.**
     - The type of widget.
-    - Examples: \"modal\", \"protection report\", \"affiliate notification\", \"help icon\".
-    - *Avoid "button" or "link". If you want to track a link or button use \`cta_click\`.*
+    - Examples: "modal", "protection report", "affiliate notification", "help icon".
+    - *Avoid "button" or "link". If you want to track a link or button use `cta_click`.*
 
 - action
     - **Required.**
     - The thing that happened.
-    - Examples: \"open\", \"accept\", \"timeout\", \"vote up\".
+    - Examples: "open", "accept", "timeout", "vote up".
     - *Avoid "click". If you want to track a click use \`cta_click\`.*
 
 - text
     - How is this action labeled to the user?
-    - Examples: \"Okay\", \"Check your protection report\", \"Get the app\"
+    - Examples: "Okay", "Check your protection report", "Get the app"
 
 - name
     - Give the widget a name.
-    - You probably only need this optional attribute if the ``text`` value is not enough to tell the widgets apart.
+    - You probably only need this optional attribute if the `text` value is not enough to tell the widgets apart.
     - This can help you group actions from the same widget, or make it easier to find the widget in the reports.
     - The dashes are not required but they\'re allowed if you want to match the element class or ID.
-    - Examples: \"dad-joke-banner\", \"focus-qr-code\", \"Join Firefox Modal\"
+    - Examples: "dad-joke-banner", "focus-qr-code", "Join Firefox Modal"
 
 - non_interaction (boolean)
     - True if the action was triggered by something other than a user gesture.

docs/docs/checklists/analytics.md

@@ -0,0 +1,36 @@
+## Links `<a>`
+
+- [ ] Has a `data-cta-text` OR `data-link-text`
+    - [ ] 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 of the same `data-cta-text` include `data-cta-position`
+    - [ ] Does not have both `data-cta-text` and `data-link-text`
+- [ ] If linking to another Mozilla property include `utm` params
+- Download button:
+    - [ ] Use the appropriate helper, don't hard code 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, see above
+- Not a download button:
+    - [ ] [`widget_action` reporting in the dataLayer](https://mozilla.github.io/bedrock/attribution/0001-analytics/#widget-action)
+
+# QR Codes
+
+-  [ ] Use the `qr_code` helper
+-  [ ] Use the apps store redirects if applicable - 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
+    - Mozilla Accounts form
+    - Newsletter subscribe
+    - Self-hosted videos
+    - Send to Device
+    - Social Share
+    - VPN subscribe button
+    - Widget Action

docs/docs/checklists/css.md

@@ -0,0 +1,23 @@
+## Responsive
+
+- [ ] CSS written mobile-first
+- [ ] In extremely wide viewports, the edges of backgrounds are not visible
+- [ ] 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
+- [ ] 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 and design tokens when available
+- [ ] No commented out code
+
+
+## Localization
+
+- [ ] Test in a RTL language (You may find the [Pseudolocalize addon](https://addons.mozilla.org/en-US/firefox/addon/pseudolocalize/) helpful.)
+- [ ] BIDI mixin used for any properties which include `left` or `right`
+- [ ] BIDI mixin used for any properties which assume LTR (e.g. `background-position`, shorthands like `border-width`)
+- [ ] BIDI mixin used for any values which include `left` or `right`

docs/docs/checklists/experiments.md

@@ -0,0 +1,23 @@
+- [ ] Consent is respected
+- [ ] There is a switch to disable it
+- [ ] If there are other conditions to run, bundle the display logic into one variable
+    - i.e. `is_enabled = switch('switchname') && geo=US && lang.startswith('en')`
+- [ ] Test all variations
+- [ ] Test an unexpected variation
+- [ ] With the experiment disabled experiment variations do not load
+- [ ] With the experiment disabled experiment code is not loaded
+- [ ] If a template was added it is `noindex` and has a canonical URL
+- [ ] The events which will determine the success of the experiment are being recorded
+    - Usually in GA but sometimes Stub Attribution or FundraiseUp
+- [ ] An `experiment_view` event is reported in the DataLayer
+- [ ] No conflicting experiments
+
+# 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](https://mozilla.github.io/bedrock/abtest/#cookies-consent)
+
+
+[Experiment documentation](https://mozilla.github.io/bedrock/abtest/)

docs/docs/checklists/general.md

@@ -0,0 +1,25 @@
+## General
+
+- [ ] Works as described
+- [ ] Code makes logical sense
+- [ ] Does not have unintended side effects
+
+## Maintainable
+
+- [ ] Comments where appropriate
+
+## Manual Testing
+
+- [ ] All breakpoints checked (320, 480, 768, 1024, 1312, 1440)
+- [ ] Tested in Gecko (Firefox)
+- [ ] Tested in Webkit (Safari)
+- [ ] Tested in Chromium (Pick one: Chrome, Edge, Brave, DuckDuckGo, Ecosia...).
+- [ ] All images in a `srcset` can be loaded
+
+
+If you are new to doing code reviews you may find these resources helpful:
+
+- [How to Do Code Reviews Like a Human (Part One)](https://mtlynch.io/human-code-reviews-1/)
+- [How to Do Code Reviews Like a Human (Part Two)](https://mtlynch.io/human-code-reviews-2/)
+- [ conventional: comments ](https://conventionalcomments.org/)
+- [Code Review Anxiety Workbook](https://developer-success-lab.gitbook.io/code-review-anxiety-workbook-1)

docs/docs/checklists/html.md

@@ -0,0 +1,30 @@
+## Best Practices
+
+- [ ] Heading levels
+    - Only one h1 on the page
+    - Heading levels convey the content hierarchy.
+    - No heading levels skipped
+- [ ] Semantic HTML
+- [ ] No validation errors
+- [ ] Automated a11y tests pass
+
+
+## Images
+
+- [ ] Have alt text where applicable
+- [ ] Use image helpers
+
+
+## Links
+
+- [ ] No hard coded "en-US" in links to Mozilla sites
+- [ ] External links have rel="external noopener"
+- [ ] Destination does not 404
+
+
+## SEO
+
+- [ ] "noindex" pages should not have the canonical or hreflang tags (bug 1442331)
+- [ ] Is there an appropriate page title and description?
+- [ ] Page linked from subnav where applicable
+

docs/docs/checklists/js.md

@@ -0,0 +1,8 @@
+- [ ] Behaves as expected
+- [ ] Logs analytics events as needed
+- [ ] No errors in the console
+- [ ] No `console.log`s
+- [ ] Conditional content displays under correct conditions (mobile vs desktop, firefox vs other, signed in to account, vs signed out)
+- [ ] promise rejections are handled
+- [ ] Has a sensible fallback for when JS is disabled.
+- [ ] Code makes logical sense

docs/docs/checklists/l10n.md

@@ -0,0 +1,37 @@
+- [ ] No hard coded English strings remain in the template file
+        - Pay special attention to hidden text like `alt` and `.visually-hidden`
+- [ ] All Fluent IDs referenced exist in the Fluent file
+- [ ] No unused Fluent IDs remain in the Fluent file
+
+## String conventions:
+
+- [ ] Strings are grouped with sub headings (## followed by an empty line)
+- [ ] Mozilla brands are Fluent IDs defined in brands.ftl
+- [ ] Idioms have explanatory comments (# with no empty line after)
+- [ ] [Variables have explanatory comments](https://mozilla.github.io/bedrock/l10n/#variables)
+- [ ] [HTML elements do not have editable attributes](https://mozilla.github.io/bedrock/l10n/#html-with-attributes)
+- [ ] Text which serves as a link has a comment with the link
+
+## ID conventions:
+
+- [ ] IDs are lowercase and hyphenated (kebab case)
+- [ ] IDs are prefixed with the file name
+- [ ] IDs are versioned when altering a string
+- [ ] [Obsolete strings kept as a fallback are dated for removal 2 months in the future](https://mozilla.github.io/bedrock/l10n/#obsolete-strings)
+- [ ] Substantial changes use new IDs and do not keep misleading content as fallbacks
+
+## If a new Fluent file is added:
+
+- [ ] File is added in `/l10n/en/`
+- [ ] File begins with a comment that includes the url of the page on the dev server (### followed by an empty line)
+- [ ] File is configured in l10n/configs/pontoon.toml
+- [ ] File is associated with a template in urls.py or a view
+
+## Styleguide:
+
+There exists no formal style guide, but here are some conventions this team will enforce until we're told otherwise.
+
+- [ ] web and internet are lowercase
+- [ ] spaces around em dashes
+- [ ] sentence case for headings/titles
+- [ ] sign in/out (not log in/out)

docs/docs/checklists/media.md

@@ -0,0 +1,2 @@
+- [ ] Optimized
+- [ ] SVG contains viewbox

docs/docs/checklists/tests.md

@@ -0,0 +1,10 @@
+## Unit tests
+
+- [ ] Existing ones are updated & passing
+- [ ] Any new ones needed?
+
+## Functional tests
+
+- [ ] Existing ones are updated & passing
+- [ ] Any new ones needed?
+- [ ] Do we need to run integration tests?

docs/mkdocs.yml

@@ -129,6 +129,16 @@ nav:
     - 10. Move CI to Github Actions for Unit and Integration tests: architecture/decisions/0010-move-ci-cd-to-github-actions.md
     - 11. Use StatsD for metrics collection: architecture/decisions/0011-use-statsd-for-metrics-collection.md
     - 12. Use Wagtail CMS: architecture/decisions/0012-wagtail-for-cms.md
+  - Code Review Checklists:
+    - General: checklists/general.md
+    - HTML: checklists/html.md
+    - CSS: checklists/css.md
+    - JS: checklists/js.md
+    - Media: checklists/media.md
+    - Analytics: checklists/analytics.md
+    - Experiments: checklists/experiments.md
+    - Localization: checklists/l10n.md
+    - Tests: checklists/tests.md
   - Contentful (deprecated): contentful.md
   - External Content Cards (deprecated): content-cards.md