Skip to content

Pull Request Review Process

Carolyn MacLeod edited this page Dec 9, 2020 · 39 revisions

Overview

The types of reviews and approvals required by a request depend on the types of additions or revisions included in the pull request. Following is a description of what is covered by each review.

Editors can create a review tracker in a pull request by using the pull request review checklist.

Editorial review

Review all prose to ensure they are:

  • Clear and understandable.
  • Phrasing is consistent throughout the guide.
  • Accurately represent the meaning of the ARIA specification.
  • Consistent with W3C and APG editorial guidelines.

Functional review

Exercise the example with keyboard, mouse, and touch in Chrome, Firefox, and Safari to ensure:

  • Keyboard behaviors match the pattern documentation.
  • Mouse and touch behaviors match expected norms.

Visual design review

Review the visual presentation to assess whether look and feel is similar to typical modern implementations of the widget. Allow for adjustments to accommodate accessibility requirements in cases where most modern implementations are not sufficiently accessible.

Accessibility review

  • Review for compliance with applicable WCAG requirements.
  • Allow for assistive technology interoperability failures where the failure is due to lack of adequate support in the browser or assistive technology.
  • Test in Windows High Contrast Mode (WHCM):
    • Turn on WHCM in Windows Settings - Ease of Access - High Contrast.
    • Open the example in Edge and Firefox (note that Firefox must be started after WHCM is on).
    • Test in both High Contast Black and High Contrast White themes.
    • Make sure everything has suitable contrast and ensure that icons have at least 1:3 contrast ratio against background in both themes.

Code review

  1. Review clarity of JavaScript and CSS source based on expectations defined in the APG Code Guide.
  2. NEW REQUIREMENT: If this change is a wide sweeping change that will require contributors to update their branches (for example, the introduction of use strict; or a change in an frequently used regression test utility function), then this PR should contain in its description step by step instructions for contributors to quickly update their local PRs after the new change is merged into master. Code reviewers should make sure these instructions are there and review these instructions for clarity.

Test review

  1. Check out the appropriate working branch locally and run npm run regression-report. The regression report will check the following:

    • whether data-test-ids are missing
    • whether tests are missing
    • whether test files are name correctly
  2. Confirm the following for the new test file:

    • There is a data-test-id attributes on every row of the keyboard and attribute documentation tables in the example's HTML file.
      • if the the attribute is testable (for example, if it is implicit), then it should have test-not-required
    • There is a test for every data-test-id.
    • The test files are name correctly (prefixed by the example directory and an underscore).
  3. Read through each test case and confirm the following:

    • The test is easy to read (if the test fails in the future, the action of the test should be manually repeatable by a future programmer trying to address the bug).
    • The test appropriate tests the behavior referenced in the example page's attributes or keyboard interaction tables.
    • The test covers all boundary conditions of the describe behavior, instead of only one example of the behavoir.
    • There are two code paths for most possible interactions with the example widget. One is using a mouse (clicking on parts of the widget, such as a menu) and the other is via the keyboard (sending key events instead of clicking). Both code paths should be reasonably covered.
    • As the tester and reviewer's discretion, tests of widget behavior not describe in the example page keyboard interaction and attribute tables can be written. These tests should have data-test-id set to "test-additional-behavior" to indicate they test behavior not tracked in the tables. These intention of these test must be particularly clear as there is no additional documentation for them.
  4. Finally, run the tests and confirm they pass.

Clone this wiki locally