Skip to content

Reviewing PRs

Chris Millar edited this page Dec 7, 2022 · 11 revisions

Non-code related things to check when reviewing PRs.

  1. Is the block name intuitive for authors?
  2. Are the variant names intuitive for authors? How do they map to Consonant?
  3. Is the solution correctly using areas of concern: page, sections, blocks?
  4. Templates should never modify any DOM inside blocks.
  5. Blocks should never modify anything outside their own context.
  6. Should the block be an auto-block / link-block?
  7. Is the authoring brittle or unintuitive?
  8. Is the performance expected to be bad (i.e. embedding an outside service) or is the block expected to be performant and cause no performance regressions?
  9. Are the files organized in a way that meet current practices?
  10. Is the feature built in a way that meets the appropriate use case:
    • Should the feature be a block? (ex. Marquee)
    • Should the feature be a link block? (ex. youtube)
    • Should the feature be a... feature? (ex. interlinks)
    • Should the feature be built as part of section metadata?
  11. Does the PR support fragments correctly?
    • Ensure JSON calls do not happen more than necessary (placeholders)
    • Ensure feature parity between page & fragment use.
  12. Does the PR correctly support contentRoot & codeRoot for consumers that exist in sub-directories (/pages/)
  13. Does the PR correctly support language-based sites (blog.adobe.com) as well as region based sites (business.adobe.com)
Clone this wiki locally