Skip to content

Commit

Permalink
Merge pull request #9698 from Expensify/flo_update_pr_template
Browse files Browse the repository at this point in the history
Update PR template to be more explicit about expectations to check off everything
  • Loading branch information
chiragsalian authored Jul 8, 2022
2 parents 2022bd1 + 4cc0226 commit 0fcc492
Showing 1 changed file with 20 additions and 14 deletions.
34 changes: 20 additions & 14 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all
- [ ] Android / Chrome
- [ ] MacOS / Chrome
- [ ] MacOS / Desktop
- [ ] I verified there are no console errors (if theres a console error not related to the PR, report it or open an issue for it to be fixed)
- [ ] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
- [ ] I followed proper code patterns (see [Reviewing the code](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md#reviewing-the-code))
- [ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. `toggleReport` and not `onIconClick`)
- [ ] I verified that comments were added to code that is not self explanatory
- [ ] I verified that any new or modified comments were clear, correct English, and explained why the code was doing something instead of only explaining what the code was doing.
- [ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- [ ] I verified any copy / text shown in the product was added in all `src/languages/*` files
- [ ] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- [ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named index.js. All platform-specific files are named for the platform the code supports as outlined in the README.
- [ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- [ ] I verified the JSDocs style guidelines (in [`STYLE.md`](https://github.com/Expensify/App/blob/main/docs/STYLE.md#jsdocs)) were followed
- [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- [ ] I followed the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md)
Expand All @@ -74,17 +74,21 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all
- [ ] For Class Components, any internal methods passed to components event handlers are bound to `this` properly so there are no scoping issues (i.e. for `onClick={this.submit}` the method `this.submit` should be bound to `this` in the constructor)
- [ ] Any internal methods bound to `this` are necessary to be bound (i.e. avoid `this.submit = this.submit.bind(this);` if `this.submit` is never passed to a component event handler like `onClick`)
- [ ] All JSX used for rendering exists in the render method
- [ ] The component has the minimum amount of code necessary for its purpose and it is
- [ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- [ ] If a new CSS style is added I verified that:
- [ ] A similar style doesnt already exist
- [ ] The style cant be created with an existing [StyleUtils](https://github.com/Expensify/App/blob/main/src/styles/StyleUtils.js) function (i.e. `StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG`)
- [ ] A similar style doesn't already exist
- [ ] The style can't be created with an existing [StyleUtils](https://github.com/Expensify/App/blob/main/src/styles/StyleUtils.js) function (i.e. `StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG`)
- [ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like `Avatar` is modified, I verified that `Avatar` is working as expected in all cases)
- [ ] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

- [ ] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

<details>
<summary><h4>PR Reviewer Checklist</h4></summary>
<summary><h4>PR Reviewer Checklist</h4>

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
</summary>

- [ ] I have verified the author checklist is complete (all boxes are checked off).
- [ ] I verified the correct issue is linked in the `### Fixed Issues` section above
- [ ] I verified testing steps are clear and they cover the changes made in this PR
- [ ] I verified the steps for local testing are in the `Tests` section
Expand All @@ -99,17 +103,18 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all
- [ ] Android / Chrome
- [ ] MacOS / Chrome
- [ ] MacOS / Desktop
- [ ] I verified there are no console errors (if theres a console error not related to the PR, report it or open an issue for it to be fixed)
- [ ] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
- [ ] I verified proper code patterns were followed (see [Reviewing the code](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md#reviewing-the-code))
- [ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. `toggleReport` and not `onIconClick`).
- [ ] I verified that comments were added to code that is not self explanatory
- [ ] I verified that any new or modified comments were clear, correct English, and explained why the code was doing something instead of only explaining what the code was doing.
- [ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- [ ] I verified any copy / text shown in the product was added in all `src/languages/*` files
- [ ] I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- [ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named index.js. All platform-specific files are named for the platform the code supports as outlined in the README.
- [ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- [ ] I verified the JSDocs style guidelines (in [`STYLE.md`](https://github.com/Expensify/App/blob/main/docs/STYLE.md#jsdocs)) were followed
- [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- [ ] I verified that this PR follows the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/docs/PR_REVIEW_GUIDELINES.md)
- [ ] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like `Avatar`, I verified the components using `Avatar` have been tested & I retested again)
- [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- [ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- [ ] If a new component is created I verified that:
Expand All @@ -122,12 +127,13 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all
- [ ] For Class Components, any internal methods passed to components event handlers are bound to `this` properly so there are no scoping issues (i.e. for `onClick={this.submit}` the method `this.submit` should be bound to `this` in the constructor)
- [ ] Any internal methods bound to `this` are necessary to be bound (i.e. avoid `this.submit = this.submit.bind(this);` if `this.submit` is never passed to a component event handler like `onClick`)
- [ ] All JSX used for rendering exists in the render method
- [ ] The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
- [ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- [ ] If a new CSS style is added I verified that:
- [ ] A similar style doesnt already exist
- [ ] The style cant be created with an existing [StyleUtils](https://github.com/Expensify/App/blob/main/src/styles/StyleUtils.js) function (i.e. `StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG`)
- [ ] A similar style doesn't already exist
- [ ] The style can't be created with an existing [StyleUtils](https://github.com/Expensify/App/blob/main/src/styles/StyleUtils.js) function (i.e. `StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG`)
- [ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like `Avatar` is modified, I verified that `Avatar` is working as expected in all cases)
- [ ] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
- [ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

</details>

Expand Down

0 comments on commit 0fcc492

Please sign in to comment.