generated from adobe/aem-boilerplate
-
Notifications
You must be signed in to change notification settings - Fork 174
Reviewing PRs
Chris Millar edited this page Oct 18, 2023
·
11 revisions
- Is there a JIRA ticket?
- Does the PR follow the 1x1x1 rule? 1 PR = 1 JIRA = 1 Issue. It is strongly encouraged that PRs are small in scope.
- Is the block name intuitive for authors?
- Are the variant names intuitive for authors? How do they map to Consonant?
- Is the solution correctly using areas of concern: page, sections, blocks?
- Templates should never modify any DOM inside blocks.
- Blocks should never modify anything outside their own context.
- Should the block be an auto-block / link-block?
- Is the authoring brittle or unintuitive?
- 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?
- Are the files organized in a way that meet current practices?
- 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?
- Does the PR support fragments correctly?
- Ensure JSON calls do not happen more than necessary (placeholders)
- Ensure feature parity between page & fragment use.
- Does the PR correctly support contentRoot & codeRoot for consumers that exist in sub-directories (/pages/)
- A feature should not blindly use
/
in front of paths because many consumers are sub-directories -/acrobat
,/pages
. - In most cases,
config.locale.contentRoot
is the most preferred prefix while content that is considered region-agnostic can usecontentRoot
.
- A feature should not blindly use
- Does the PR correctly support language-based sites (blog.adobe.com) as well as region based sites (business.adobe.com)
- Did the feature add anything to utils.js and it shouldn't have?
- Only if something is needed for every LCP should it be inside utils.js.
- Are there any unexpected console logs?
- Is the code in the right place (no unnecessary code in utils.js etc)