From e9c9b4ac322953cf971b9e3221e025c8df50943f Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Tue, 5 Jul 2022 16:23:29 +0100 Subject: [PATCH 1/7] replace pretty quotes with normal quotes --- .github/PULL_REQUEST_TEMPLATE.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index f5a45bd6f631..31dd0bed6b00 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -50,7 +50,7 @@ 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 there’s 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 @@ -76,8 +76,8 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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 - [ ] If a new CSS style is added I verified that: - - [ ] 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`) + - [ ] 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. @@ -99,7 +99,7 @@ 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 there’s 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 @@ -124,8 +124,8 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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 - [ ] If a new CSS style is added I verified that: - - [ ] 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`) + - [ ] 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. From b940ee47df104c7a7176a92f451ef1ea648624b0 Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Tue, 5 Jul 2022 16:26:10 +0100 Subject: [PATCH 2/7] Add missing checklist item --- .github/PULL_REQUEST_TEMPLATE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 31dd0bed6b00..b7547b640273 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -110,6 +110,7 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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: From f3863e42e206e89425cfef2bd68f3be841baefd2 Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Tue, 5 Jul 2022 16:26:42 +0100 Subject: [PATCH 3/7] replace pretty quotes with normal quotes --- .github/PULL_REQUEST_TEMPLATE.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index b7547b640273..9dd90c6e900c 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -54,10 +54,10 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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) @@ -103,10 +103,10 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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) From f7494bf47b6250461ff30797a25978f6d0ae1b4e Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Tue, 5 Jul 2022 16:27:36 +0100 Subject: [PATCH 4/7] fix text so that we can read it --- .github/PULL_REQUEST_TEMPLATE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 9dd90c6e900c..94af92eabdf1 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -74,7 +74,7 @@ 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 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`) @@ -123,7 +123,7 @@ 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 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`) From ff326dec3b5b8674bc3a820e5a4db529c3b5b3f7 Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Tue, 5 Jul 2022 16:29:10 +0100 Subject: [PATCH 5/7] Clarify what to do with the C+ checklist --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 94af92eabdf1..1b0d02bf19e3 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -83,7 +83,7 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all
-

PR Reviewer Checklist

 +

PR Reviewer Checklist to copy/paste into a new comment after the author checklist is completed, and to be completed by a Contributor+

 - [ ] 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 From 32cfb97e5a8dee1db0be8207a84c5259d8ebcdee Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Tue, 5 Jul 2022 16:30:54 +0100 Subject: [PATCH 6/7] Add clarity around what to do with items that do not apply --- .github/PULL_REQUEST_TEMPLATE.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 1b0d02bf19e3..9088da5e0ee1 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -80,11 +80,12 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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.

PR Reviewer Checklist to copy/paste into a new comment after the author checklist is completed, and to be completed by a Contributor+

 +- [ ] 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 @@ -129,6 +130,7 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] 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.
From 4cc02262d88f6124210c8d84d3a528f0362b635d Mon Sep 17 00:00:00 2001 From: Florent De Neve Date: Fri, 8 Jul 2022 16:12:11 +0100 Subject: [PATCH 7/7] move note outside of the header --- .github/PULL_REQUEST_TEMPLATE.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 9088da5e0ee1..a8dc334ecd5f 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -83,7 +83,10 @@ This is a checklist for PR authors & reviewers. Please make sure to complete all - [ ] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.
-

PR Reviewer Checklist to copy/paste into a new comment after the author checklist is completed, and to be completed by a Contributor+

 +

PR Reviewer Checklist

+ +The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed + - [ ] 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