Skip to main content

Code Reviews

Frontend​

We will use the following guidelines when reviewing merge requests (MRs) for the frontend.

This is a working document that will change over time.

Ultimately these rules should make our lives easier - with this in mind, we encourage reviewers to use best judgment when evaluating each MR. Situations/edge cases will arise where strict, pedantic adherance to the list below might not actually make our lives easier - in those cases, we will be reasonable and flexible where appropriate.

Required​

Standard expectations​

  • "Acceptance Criteria" have been met (any deviations not already added to the AC have been agreed upon and outlined in the MR)
  • Tests relating to the issue are passing
  • No regressions (this includes not breaking SSR)

Comments and names​

  • ALL new components and services have at least one descriptive class-level comment
  • ALL complex functions should have descriptive explanations
  • Explanatory comments provided for anything hacky, out of the ordinary, non-intuitive, or requires looking into other files to get context (e.g. @Inputs/@Outputs)
  • Clear, unambiguous BEM CSS class and Angular selector names

Testing​

  • New components must have a testbed built
  • Unit testing included where feasible
  • Changes have been tested against sandboxes by the developer before draft status is removed, unless otherwise agreed (in that case, the details of the agreement should be included within the "Testing Considerations" section of the MR template)

Ongoing maintenance​

  • GitLab issues created and linked for any TODOs and/or failing tests that are unrelated to the issue

Very nice to have​

  • Sub-class-level comments for every function
  • Typings are specific, targeted, and explicit (as long as they are in scope)
  • i18n tags added to text that will need translating
  • New files are created in logical places
  • Re-use code wherever feasible
  • Make re-usable code where appropriate
  • Create tests are unlikely to break from minor changes (e.g. Use data-refs as locators. Do NOT use class names or specific text strings)
  • Data-refs included on buttons for analytics tracking
  • Properly encapsulate your functions and variables
  • Unused code is deleted (unless we anticipate needing it in future)
  • Avoid using ::ng-deep where feasible