From 73466bc1a8fb4059c9d45c01d578a9caca9033d3 Mon Sep 17 00:00:00 2001 From: Gauthier Petetin Date: Mon, 18 Sep 2023 23:04:18 +0200 Subject: [PATCH] feat(templates): new templates for issues and PRs (#20651) * feat(templates): new templates for issues and PRs We've discussed with stakeholders from Extension and Mobile teams how we could align templates for issues and PRs on the different repos, starting with metamask-extension and metamask-mobile repos. This is the proposal we landed on after our first workshops. * fix(templates): links were not working * fix(templates): tiny adjustments * fix(templates): add expected behavior section to bug report template This change was suggested by Nicolas Massart. * fix(templates): add a section to indicate what issue is fixed by the PR This change was suggested by Nicolas Massart. * fix(guidelines): add a coding guideline regarding the use of useEffect * fix(guidelines): rename guidelines folder * fix(contributing.md): update link to good first issues * fix(contributing.md): recommend contributors to read coding guidelines * fix(guidelines): coding guidelines for component file structure are different in extension * fix(guidelines): TypeScript shall be written in camelcase * fix(guidelines): simplify component naming guideline Comment from George Marshall: Because we use kebab-case for file names but retain PascalCase for component and enum names it may make sense to reword this sentence * fix(guidelines): tiny changes to adapt mobile guidelines to extension context * feat(guidelines): add links to contributor docs * fix(issue template): make instruction clearer for external contributors --- .github/CONTRIBUTING.md | 3 +- .github/ISSUE_TEMPLATE/bug-report.yml | 34 +++++- .github/ISSUE_TEMPLATE/general-issue.yml | 105 ++++++++++++++++++ .github/PULL_REQUEST_TEMPLATE.md | 53 --------- .github/guidelines/CODING_GUIDELINES.md | 64 +++++++++++ .../{ => guidelines}/LABELING_GUIDELINES.md | 9 +- .github/pull-request-template.md | 46 ++++++++ 7 files changed, 252 insertions(+), 62 deletions(-) create mode 100644 .github/ISSUE_TEMPLATE/general-issue.yml delete mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 .github/guidelines/CODING_GUIDELINES.md rename .github/{ => guidelines}/LABELING_GUIDELINES.md (83%) create mode 100644 .github/pull-request-template.md diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 74131102ecea..a7d720258e0b 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -6,13 +6,14 @@ If you're submitting code to MetaMask, there are some simple things we'd appreci Before taking the time to code and implement something, feel free to open an issue and discuss it! There may even be an issue already open, and together we may come up with a specific strategy before you take your precious time to write code. -There are also plenty of open issues we'd love help with. Search the [`good first issue`](https://github.com/MetaMask/metamask-extension/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) label, or head to Gitcoin and earn ETH for completing projects we've posted bounties on. +There are also plenty of open issues we'd love help with. Search the [`good first issue`](https://github.com/MetaMask/metamask-extension/contribute) label, or head to Gitcoin and earn ETH for completing projects we've posted bounties on. If you're picking up a bounty or an existing issue, feel free to ask clarifying questions on the issue as you go about your work. ### Submitting a pull request When you're done with your project / bugfix / feature and ready to submit a PR, there are a couple guidelines we ask you to follow: +- [ ] **Make sure you followed our [`coding guidelines`](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md)**: These guidelines aim to maintain consistency and readability across the codebase. They help ensure that the code is easy to understand, maintain, and modify, which is particularly important when working with multiple contributors. - [ ] **Test it**: For any new programmatic functionality, we like unit tests when possible, so if you can keep your code cleanly isolated, please do add a test file to the `tests` folder. - [ ] **Add to the CHANGELOG**: Help us keep track of all the moving pieces by adding an entry to the [`CHANGELOG.md`](https://github.com/MetaMask/metamask-extension/blob/develop/CHANGELOG.md) with a link to your PR. - [ ] **Meet the spec**: Make sure the PR adds functionality that matches the issue you're closing. This is especially important for bounties: sometimes design or implementation details are included in the conversation, so read carefully! diff --git a/.github/ISSUE_TEMPLATE/bug-report.yml b/.github/ISSUE_TEMPLATE/bug-report.yml index 90450a3953aa..df752860bfb1 100644 --- a/.github/ISSUE_TEMPLATE/bug-report.yml +++ b/.github/ISSUE_TEMPLATE/bug-report.yml @@ -16,17 +16,30 @@ body: id: what-happened attributes: label: Describe the bug - description: What happened? What did you expect to happen? Please include screenshots if applicable! - placeholder: Tell us what you see! + description: What happened? + placeholder: A clear and concise description of what the bug is validations: required: true + - type: textarea + id: expected-behavior + attributes: + label: Expected behavior + description: What did you expect to happen? + - type: textarea + id: screenshot + attributes: + label: Screenshots + description: Please include screenshots if applicable! - type: textarea id: reproduce attributes: label: Steps to reproduce - description: List all steps needed to reproduce the problem - value: | - 1. + description: "List all steps needed to reproduce the problem:" + placeholder: | + 1. Go to '...' + 2. Click on '...' + 3. Scroll down to '...' + 4. See error validations: required: true - type: textarea @@ -96,3 +109,14 @@ body: attributes: label: Additional context description: Add any other context about the problem here, e.g. related issues, additional error messages or logs, or any potentially relevant details about the environment or situation the bug occurred in. + - type: textarea + id: severity + attributes: + label: Severity + description: | + To be added after bug submission by internal support / PM: + placeholder: | + - How critical is the impact of this bug on a user? + - Add stats if available on % of customers impacted + - Is this visible to all users? + - Is this tech debt? diff --git a/.github/ISSUE_TEMPLATE/general-issue.yml b/.github/ISSUE_TEMPLATE/general-issue.yml new file mode 100644 index 000000000000..f23b47082e5a --- /dev/null +++ b/.github/ISSUE_TEMPLATE/general-issue.yml @@ -0,0 +1,105 @@ +name: General issue +description: For any issues +labels: [] +assignees: [] + +body: + - type: markdown + attributes: + value: | + ## **Description** + + - type: textarea + id: description + attributes: + label: What is this about? + placeholder: Describe the issue here. + validations: + required: true + + - type: markdown + attributes: + value: | + ## **Scenario (for user stories only)** + + - type: textarea + id: scenario + attributes: + label: Scenario + placeholder: | + - GIVEN a user is in x state + - WHEN a user does x + - AND a user does x + - THEN x should occur + + - type: markdown + attributes: + value: | + ## **Design (for user stories only)** + + - type: textarea + id: design + attributes: + label: Design + placeholder: If available, provide a link to the design (e.g., Figma) or screenshots. If you're an external contributor and don't have access to the design, feel free to skip this step or provide any visual representation of the issue if possible. + + - type: markdown + attributes: + value: | + ## **Technical Details (for technical tasks only)** + + - type: textarea + id: technical-details + attributes: + label: Technical Details + placeholder: | + - Implementation details + - Insight to what needs to be done + - Etc. + + - type: markdown + attributes: + value: | + ## **[Threat Modeling Framework](https://github.com/adamshostack/4QuestionFrame) (for technical tasks only)** + + - type: textarea + id: threat-modeling + attributes: + label: Threat Modeling Framework + placeholder: | + - What are we working on? What does this aim to solve? + - What can go wrong? + - What are we going to do about it? + - Did we do a good job? + + - type: markdown + attributes: + value: | + ## **Acceptance Criteria** + + - type: textarea + id: acceptance-criteria + attributes: + label: Acceptance Criteria + placeholder: | + - Are metrics required? + - Are translations required? + - Cases to satisfy + - XYZ should work + - Etc. + + - type: markdown + attributes: + value: | + ## **References** + + - type: textarea + id: references + attributes: + label: References + placeholder: | + - References go here. + - Screenshots. + - Issue numbers. Links. + - Slack threads. + - Etc. diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md deleted file mode 100644 index 8f4c7ba9eac2..000000000000 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ /dev/null @@ -1,53 +0,0 @@ -## Explanation - - - -## Screenshots/Screencaps - - - -### Before - - - -### After - - - -## Manual Testing Steps - - - -## Pre-merge author checklist - -- [ ] I've clearly explained: - - [ ] What problem this PR is solving - - [ ] How this problem was solved - - [ ] How reviewers can test my changes -- [ ] Sufficient automated test coverage has been added - -## Pre-merge reviewer checklist - -- [ ] Manual testing (e.g. pull and build branch, run in browser, test code being changed) -- [ ] PR is linked to the appropriate GitHub issue -- [ ] **IF** this PR fixes a bug in the release milestone, add this PR to the release milestone - -If further QA is required (e.g. new feature, complex testing steps, large refactor), add the `Extension QA Board` label. - -In this case, a QA Engineer approval will be be required. diff --git a/.github/guidelines/CODING_GUIDELINES.md b/.github/guidelines/CODING_GUIDELINES.md new file mode 100644 index 000000000000..a59e3fd71fa5 --- /dev/null +++ b/.github/guidelines/CODING_GUIDELINES.md @@ -0,0 +1,64 @@ +# MetaMask Coding Guidelines + +### 1. New Code Should be TypeScript +- New components and utilities should be written in TypeScript and enforce typing. +- Existing code should be refactored into TypeScript where time allows. If you are replacing a component, use TypeScript. +- Follow contributor doc [TypeScript Guidelines](https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md). + +### 2. Using Functional Components and Hooks Instead of Classes +- Use functional components and hooks as they result in more concise and readable code compared to classes. + +### 3. Organize Files Related to the Same Component in One Folder +- An example of a component file structure: + +```.tsx +avatar-account +├── avatar-account.stories.tsx +├── avatar-account.scss +├── avatar-account.test.tsx +├── avatar-account.tsx +├── avatar-account.types.ts +├── README.md +├── __snapshots__ +│   └── avatar-account.test.tsx.snap +└── index.ts +``` + +### 4. Follow Naming Conventions +- You should always use PascalCase when naming components. For example: *TextField*, *NavMenu*, and *SuccessButton*. +- Use camelCase for functions declared inside components like *handleInput()* or *showElement()*. +- When creating hooks use *withHookName()*. + +### 5. Avoid Repetitive Code +- If you notice you are writing duplicated code or components, convert it into a component, utility functions or hooks that can be reused. Do this with [scalable intention](https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction). + +### 6. Component Optimization +- For functional components, instead of having large return statements, breaking the component down into smaller sub-components. +- Use memoizing techniques where possible. Utilize the `useMemo` hook for values and `useCallback` for functions. Follow recommended React guidance. +- Use the useEffect hook for performing side effects like data fetching or DOM manipulation after the component has rendered. However, use it judiciously as unnecessary effects can complicate code and affect performance. For a deeper understanding, we recommend reading [this article](https://react.dev/learn/you-might-not-need-an-effect). + +### 7. Use Object Destructuring For Props +- Instead of passing the props object, use object destructuring to pass the prop name. This discards the need to refer to the props object each time you need to use it. + +```tsx +import React from 'react'; +const MyComponent = ({ id }) => { + return
; +}; + +``` + +### 8. Document Each Component/Utility +- New utility functions should be documented [TSDoc](https://tsdoc.org) commenting format. +- Referencing our component docs. +- If applicable add URL to online resources if they are meaningful for the component/method. + +### 9. Write Tests for Each Component/Utility +- Write tests for the components you create as it reduces the possibilities of errors. Testing ensures that the components are behaving as you would expect. In this project Jest is used, and it provides an environment where you can execute your tests. +- Follow the contributor docs [Unit Testing Guidelines](https://github.com/MetaMask/contributor-docs/blob/main/docs/unit-testing.md). + +### 10. External packages should be well maintained +- New packages should only be integrated if the application doesn’t have the existing functionality and it cannot be added by implementing a small utility function. Use the https://snyk.io/advisor/ to assess the popularity, maintainability and security analysis. The package must be in good standing to be added to the project. +- Update existing dependencies when you notice they are out of date. + +[Source](https://www.makeuseof.com/must-follow-react-practices/) diff --git a/.github/LABELING_GUIDELINES.md b/.github/guidelines/LABELING_GUIDELINES.md similarity index 83% rename from .github/LABELING_GUIDELINES.md rename to .github/guidelines/LABELING_GUIDELINES.md index 27247b7dfefb..ec7ad636c729 100644 --- a/.github/LABELING_GUIDELINES.md +++ b/.github/guidelines/LABELING_GUIDELINES.md @@ -1,17 +1,20 @@ # PR Labeling Guidelines To maintain a consistent and efficient development workflow, we have set specific label guidelines for all pull requests (PRs). Please ensure you adhere to the following instructions: -### Mandatory Labels: +### Mandatory team labels: - **Internal Developers**: Every PR raised by an internal developer must include a label prefixed with `team-` (e.g., `team-extension-ux`, `team-extension-platform`, etc.). This indicates the respective internal team responsible for the PR. - **External Contributors**: PRs from contributors outside the organization must have the `external-contributor` label. It's essential to ensure that PRs have the appropriate labels before they are considered for merging. -### Prohibited Labels: +### Optional QA labels: +- **needs-qa**: If the PR includes a new features, complex testing steps, or large refactors, this label must be added to indicated PR requires a full manual QA prior being merged and added to a release. + +### Labels prohibited when PR needs to be merged: Any PR that includes one of the following labels can not be merged: -- **needs-qa**: The PR requires a full manual QA prior to being added to a release. +- **needs-qa**: The PR requires a full manual QA prior to being merged and added to a release. - **QA'd but questions**: The PR has been checked by QA, but there are pending questions or clarifications needed on minor issues that were found. - **issues-found**: The PR has been checked by QA or other reviewers, and appeared to include issues that need to be addressed. - **need-ux-ds-review**: The PR requires a review from the User Experience or Design System teams. diff --git a/.github/pull-request-template.md b/.github/pull-request-template.md new file mode 100644 index 000000000000..fcdebbcb5215 --- /dev/null +++ b/.github/pull-request-template.md @@ -0,0 +1,46 @@ +## **Description** +_Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions:_ +_1. What is the reason for the change?_ +_2. What is the improvement/solution?_ + +## **Manual testing steps** + +_1. Step1:_ +_2. Step2:_ +_3. ..._ + +## **Screenshots/Recordings** + +_If applicable, add screenshots and/or recordings to visualize the before and after of your change._ + +### **Before** + +_[screenshot]_ + +### **After** + +_[screenshot]_ + +## **Related issues** + +_Fixes #???_ + +## **Pre-merge author checklist** + +- [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). +- [ ] I've clearly explained: + - [ ] What problem this PR is solving. + - [ ] How this problem was solved. + - [ ] How reviewers can test my changes. +- [ ] I’ve indicated what issue this PR is linked to: Fixes #??? +- [ ] I’ve included tests if applicable. +- [ ] I’ve documented any added code. +- [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). +- [ ] I’ve properly set the pull request status: + - [ ] In case it's not yet "ready for review", I've set it to "draft". + - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". + +## **Pre-merge reviewer checklist** + +- [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). +- [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.