Pull Requests and Releases

Opening Pull Requests

When writing PRs, it is ideal to have the following in the description to make it easier to review:

• Summary: A brief description of the changes
• Link to the zenhub or github issue
• "How to test" section
• Screenshots or Screenrecording if any

Reviewing pull requests

• Read description and scope of the ticket to guide you in your code review
• Provide clear, constructive, actionable feedback. Use the suggestion feature instead of typing out a sentence if possible
• Test the code following the test instructions, and think of other ways the code could behave if you can think of any
• Check for functionality, visual changes, behaviour, and security
• When approving a pull request, include the things you tested for (if possible)

When to require one or two reviews? For a team of 3 developers, our github settings only require 1 approving review to merge. Most of the time, 1 thorough review is enough, especially for these scenarios:

• Style changes that can be quickly checked and tested by the reviewer visually
• Small updates that don't impact component behaviour
• Low risk of impact to users

There are situations where having 2 approving reviews may help, such as:

• Major change in behaviour
• A change that does not have automated tests, it could pose a higher risk that we may not anticipate
• Major refactors, especially if tests did not exist beforehand
• Most releases, unless doing a patch release
• Cross-team changes, or changes that involve or may impact design or other teams

Conventional commits

We utilize Release Please to automate our releases for all of our products and services. Automated releases are made possible by the tool reading the commits using the conventional commits spec so it knows when to create a patch, minor or major release following semantic versioning or SemVer.

Here's a guide to help think of which commit type might fit best:

• fix: When the PR is a fix to a bug. A bug fix is defined as an internal change that fixes incorrect behaviour. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens. A type fix will correspond to a PATCH in SemVer.
• feat: When the PR adds a new feature that is backwards compatible. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens. This will correspond to a MINOR in SemVer.
• remove: Only use this for the tokens and utility packages for now. It is useful to signal the removal of tokens, or CSS classes within the changelog while the the products are still in an alpha phase. ⚠️ Avoid removing things as much as possible, we will not be using this once we're not in alpha anymore as it is backwards incompatible.
• refactor: When the PR is a code improvement that (usually) addresses technical debt and code smells. Use this type for any substantial code changes that do not add functionality or fix bugs but optimize and improve the internal structure, readability, or maintainability of the codebase. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens. This will correspond to a MINOR in SemVer.
• perf: When the PR has code changes that improve performance of the product. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens. This will correspond to a MINOR in SemVer.
• build: When the PR contains changes to the build process of the product. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens.
• ci: When the PR contains changes to the ci/cd pipeline, infrastructure, or any workflow changes. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens.
• docs: When the PR only contains changes to the documentation. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens.
• style: We have removed this since a majority of our changes fall under this type. But typically, this is used to denote minor changes to the styles that either enhances or impacts the visual display and render of the product, which would bump the SemVer up to a MINOR version. There should be no external API changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens.
• test: When the PR only contains code that adds tests. This is not a common PR title, it is mostly used for single commits. There should be no external changes in the product, no new properties/classes/tokens, or removal of properties/classes/tokens.
• chore: For any other commits that do not quite fit in any of the above, and are changes to the internal workings of the system. Most maintenance tasks will fall under this type. We typically do not release PRs that are in this category, but they are included in the next version bump release, but won't show up on the CHANGELOG or release notes by default.