Skip to content

Commit

Permalink
(BSR) docs(PR): add best practices to template
Browse files Browse the repository at this point in the history
  • Loading branch information
cgerrard-pass committed Nov 20, 2024
1 parent c3be064 commit 41c7f09
Showing 1 changed file with 27 additions and 0 deletions.
27 changes: 27 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ I have:
- [ ] Written **unit tests** native (and web when implementation is different) for my feature.
- [ ] Added a **screenshot** for UI tickets or deleted the screenshot section if no UI change
- [ ] If my PR is a bugfix, I add the link of the "résolution de problème sur le bug" [on Notion][1]
- [ ] I am aware of all the best practices and respected them.

## Screenshots

Expand All @@ -27,3 +28,29 @@ I have:

[1]: https://www.notion.so/passcultureapp/R-solution-de-probl-mes-sur-les-bugs-5dd6df8f6a754e6887066cf613467d0a
[2]: https://www.notion.so/passcultureapp/cb45383351b44723a6f2d9e1481ad6bb?v=10fe47258701423985aa7d25bb04cfee&pvs=4

## Best Practices

<details>
<summary>Click to expand</summary>

- Remove type assertions with `as` (type assertions are removed at compile-time, there is no runtime checking associated with a type assertion. There won’t be an exception or `null` generated if the type assertion is wrong). In certain cases `as const` is acceptable (for example when defining readonly arrays/objects). **MUST BE DISCUSSED: using `as` in tests**
- Remove bypass type checking with `any` (when you want to accept anything because you will be blindly passing it through without interacting with it, you can use `unknown`). **MUST BE DISCUSSED: when defining functions that can take literally anything, it would seem that it is not practical to use `unknown`. Also we should discuss the usage on `any` in tests.**
- Remove non-null assertion operators (just like other type assertions, this doesn’t change the runtime behavior of your code, so it’s important to only use `!` when you know that the value can’t be `null` or `undefined`).
- When creating a new component, start by creating it in StoryBook. **MUST BE DISCUSSED: what do we consider a component? Should all components be in StoryBook?**
- When creating a new logic, start by writing tests (TDD). **MUST BE DISCUSSED**
- Remove all `@ts-expect-error` and `@eslint-disable`. If not avoidable, specify why with a comment. **MUST BE DISCUSSED**
- Remove all warnings, and "normal errors" (`yarn test:lint`, `yarn test:types`, `yarn start:web`...). **MUST BE DISCUSSED**
- Write tests for existing code if there were none. **MUST BE DISCUSSED**
- Use `gap` instead of `<Spacer.Column />` or `<Spacer.Row />` when it is more optimal. **MUST BE DISCUSSED**

Test specific:

- Mock external calls, but don't mock internal parts of our code. **MUST BE DISCUSSED**
- When you see a local variable that is over-witten in every test, mock it.
- Prefer `user` to `fireEvent`.
- Replace `await waitFor(/* ... */)` or `await act(async () => {})` by `await screen.findByTruc()`. **MUST BE DISCUSSED**
- Add a snapshot test for pages and modals ONLY. **MUST BE DISCUSSED: snapshots for web?**
- Add an a11y test for the web. **MUST BE DISCUSSED**

</details>

0 comments on commit 41c7f09

Please sign in to comment.