Skip to content

Pull Request Review Guide

Benny Powers edited this page Jul 7, 2022 · 2 revisions

When reviewing pull requests for new elements, follow these steps:

  1. Changeset
  2. Conventional Commits
  3. Online Demo
  4. README
  5. Inline Documentation
    1. Slots
    2. CSS Custom Properties
    3. CSS Shadow Parts
    4. Events
    5. Class Fields
    6. Methods

Changeset

If the PR adds or updates an element, it must contain a changeset. If the PR does not contain a changeset, it won't trigger a release train, and won't update the CHANGELOG.

The changeset type should be one of:

  • patch - for bug fixes only
  • minor - for new elements, or for new APIs in an element
  • major - for breaking changes to an element or a public library API

Conventional Commits

In addition, the commit messages for the PR should reflect the changes made. RHDS follows conventional commit.

In addition to fix and feat commit types, we also use:

  • chore - for build tooling, repo management, dependency updates, etc.
  • docs - for changes to element documentation, including changes to ux.redhat.com
  • style - NOT for CSS changes, rather for thigs like linting and code formatting
  • perf - performance upgrades
  • test - unit or regression tests

If the commit is for to a particular element, it should use the unprefixed element name for scope. For example, if the commit adds a breaking feature change to <rh-jazz-hands>:

feat(jazz-hands)!: change `period` attribute to `era`

Commit messages may elaborate in the footer, but not at the expense of the changeset.

Squashing Commits

If the PR contains commits which don't follow the conventions, it should be squashed. If any one squashed commit is breaking, that must reflect in the new commit message.

Online Demo

New elements should have a demo page (under /demo) suitable for both manual testing (design review and accessibility audits) and for automated visual regression testing. In other words, the demo page should contain instances of each the element's possible initial states, e.g. via attributes, slotted content or css custom properties.

README

The element's README file should contain a terse description and a minimal example. It should not contain any boilerplate or placeholder text.

Inline Documentation

All element APIs should be documented inline. Depending on the kind of API, the means of documention may be slightly different. See the JSDoc style guide.

Slots

All slots must be documented in the element class' docblock.

CSS Custom Properties

Element authors may use "private" CSS custom properties to simplify their CSS files or accomplish other styling needs. Such properties do not need to be documented, but reviewers should verify with the PR authors that this was their intent.

All other CSS custom properties must be documented. Reviewers should scan element CSS files for variables and ensure that all public properties are documented. Where possible, existing design tokens from the Red Hat Design Tokens package should be used or aliased.

CSS Shadow Parts

All CSS shadow parts must be documented. Reviewers should scan element classes' render() method and any other class methods, getters, controllers, directives, etc., which render Shadow DOM and ensure that any rendered parts are documented.

If any shadow children export parts, those part names should be documented as well.

Events

Any events dispatched using dispatchEvent in the element's class should have a corresponding @fires docblock.

Class Fields

Every public or protected class field must have a docblock containing at least a description. If the field is decorated with @property, the attribute will automatically be added to the custom-elements manifest.

Private class fields (either ecma private fields starting with #, or fields annotated with the TypeScript private keyword) do not need docblocks, but may have them.

Methods

Like class fields, all public and protected methods should have docblocks with a description, @param, and @return tags as needed.