-
Notifications
You must be signed in to change notification settings - Fork 5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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
- Loading branch information
1 parent
62c7353
commit 73466bc
Showing
7 changed files
with
252 additions
and
62 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 <div id={id} />; | ||
| }; | ||
|
|
||
| ``` | ||
|
|
||
| ### 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/) |
9 changes: 6 additions & 3 deletions
9
.github/LABELING_GUIDELINES.md → .github/guidelines/LABELING_GUIDELINES.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |