refactor(v2): replace react-toggle with own implementation #4608
Conversation
lex111
added
the
pr: polish
|
[V2] Built with commit dcd0002 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4608--docusaurus-2.netlify.app/ |
|
[V1] Built with commit dcd0002 |
|
Size Change: +490 B (0%) Total Size: 598 kB
ℹ️ View Unchanged
|
| const { | ||
| colorMode: { | ||
| switchConfig: {darkIcon, darkIconStyle, lightIcon, lightIconStyle}, | ||
| }, | ||
| } = useThemeConfig(); | ||
| const {isClient} = useDocusaurusContext(); | ||
| const [checked, setChecked] = useState(defaultChecked); |
There was a problem hiding this comment.
Can you extract all the non-docusaurus code to a separate function and mention in a comment it's inspired from react-toggle? Mixing concerns like reading Docusaurus context and handling low-level dom code is not a good idea and will make the code less reusable once we'll have multiple themes
Also, the original comp used PureComponent so we can also use React.memo() around that newly extracted component too (the onChange is already memorized)
There was a problem hiding this comment.
It seems redundant to me to create another component as like before, I also don't understand the gist of memoizing this new child component, but okay I'll do it.
There was a problem hiding this comment.
The concept of a component (or hook) is to encapsulate a concerns.
If multiple concerns end up being mixed, you'd rather split and refactor into multiple components. Creating large components and mixing concerns does not help to make the code readable, maintainable, and is also less performant (in most cases) as you have to render the thing as a whole instead.
I'd favor readability and good encapsulation over perf in most cases.
We'll probably someday put this UI component in a Storybook and test it in isolation. We don't necessarily want to couple it to docusaurus context, as the test/storybook will now have to "mock" that context (ie it will require a provider)
There was a problem hiding this comment.
I get it, it's just that our earlier components didn't follow that pattern, although we'll need to refactor them the same way, I guess.
There was a problem hiding this comment.
yes that's the goal but no hurry for now
|
LGTM thanks 👍 |
Motivation
A lightweight and simplified alternative to react-toggle built on functional component/hooks instead on class as in original component. Also I deliberately not using touch events, because I think a mobile user can just tap on toggle to switch mode ( not sure if someone used gestures for this).
As another advantage, we get rid of another external dependency and have more control over this component.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
No breaking changes, CSS class names are same.
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)