docs: create react.md #65
Conversation
docs/react.md
Outdated
| **Components should aim to meet these requirements:** | ||
|
|
||
| - [ ] Use TypeScript | ||
| - [ ] Include Storybook page |
There was a problem hiding this comment.
Some of these seem very focused on the extension. Some other React projects (mainly Snaps stuff) don't use Storybook or e2e tests yet.
There was a problem hiding this comment.
I would strongly recommend for any project that uses UI to use storybook when possible
There was a problem hiding this comment.
Hmm, should this be qualified a bit more though? i.e. which types of components should have a Storybook file? I always thought of that as more useful for "reusable components". Whereas getting a working "entire page" component in Storybook is quite difficult and seems less useful
There was a problem hiding this comment.
Re: some React projects not using Storybook, that's fine as long as we think this guideline still applies (i.e. that it would be useful to use Storybook for them, even if we haven't gotten around to it yet). If there is some disagreement about it being useful for all React projects, maybe we can refine this a little more.
There was a problem hiding this comment.
hello 👋🏼 thanks all for the input, everyone! Sorry for the delay on this. I whipped this PR up and then set it aside to work on other items. I'm back.
updated the guidelines to improve its structure and add more input. I made an attempt to address the Storybook feedback above. I also noted how not all guidelines will be applicable depending on the respective repository.
I tried to keep it general. I considered linking specific repo guidelines and then thought against it to avoid stale docs and having less to maintain.
please let me know what you think of these updates, if the new messaging related to Storybook is okay, or if you have any more suggestions! all comments or suggestions are welcomed and appreciated
There was a problem hiding this comment.
I'm a bit of a storybook maxi 😅 I find it to be an exceptionally powerful tool for documentation and UI development, effectively bridging the gap between design and engineering while significantly streamlining the UI development, review, and iteration processes. However, I acknowledge that it might not be the perfect fit for every team. Including "(If applicable)" as you've suggested seems like a sensible approach.
There was a problem hiding this comment.
To be clear, I'm not saying we dislike Storybook. I think it's a very useful tool. We haven't implemented it because it hasn't been high on our list of priorities. 😅
There was a problem hiding this comment.
sweet, it sounds like we're on board for including Storybook pages for UI components. It's okay if some repos haven't implemented it yet or lack storybook support. I think this part of this guide covers it
If certain requirements cannot be met due to time constraints, legacy code, or other factors, contributors should use their best judgment on which requirements are most important or feasible to implement.
Thanks @georgewrmarshall for helping us refine these points! Also, I agree with @Gudahtt that we can refine this more should there be any other disagreements or concerns.
I've questioned having storybook pages for some atomic components, especially when they're only displayed in one other component. That said, I do believe it aids more than it hurts. It might make searching for storybook pages more noisy, but it could be helpful for someone quickly grasping the UI component(s) context.
As for pages, when I first started working on the extension, I found them especially helpful as I was learning to navigate our app and be exposed to various states.
docs/react.md
Outdated
| - [ ] Include Storybook page | ||
| - [ ] Include unit tests | ||
| - [ ] Include relevant e2e tests | ||
| - [ ] Avoid unnecessary re-renders |
There was a problem hiding this comment.
Would be nice to clarify some ways to do this. For example, avoiding arrow functions inside props (which happens a lot in the extension right now).
There was a problem hiding this comment.
Great call 👍🏼
what if we add links to help elaborate? I added 2 example links here 86a92d6
suggesting this to help keep the guide concise
There was a problem hiding this comment.
Here are some more good tips: https://hackernoon.com/best-practices-for-react-performance-optimization
But some small examples/explanations like @mcmire has done in the JavaScript guide could be nice too? We could aggregate the different best practices mentioned in these links into a single document.
Co-authored-by: George Marshall <george.marshall@consensys.net>
Co-authored-by: George Marshall <george.marshall@consensys.net>
Starting a guide for contributing to React repositories.
All suggestions and comments are welcomed, minus spam (: