PR for #147 Refactor components to use functional components #148
Conversation
b18050
requested review from
arvinpoddar,
asylbek09,
albertodainotti,
amandahmeng and
ydaniju
|
Would be great to split this up into smaller PRs for 2 reasons:
|
| mounted: false, | ||
| }; | ||
| } | ||
| const [language, setLanguage] = useState(getSavedLanguagePreference()); |
There was a problem hiding this comment.
If we're using React v18+, we may be able to use useSyncExternalStore, which provides a more robust method for reading data from external systems (like localStorage). I wrote a blog post on this: https://www.arvinpoddar.com/blog/syncing-local-storage-with-react-state
There was a problem hiding this comment.
Thank you for the suggestion. I'll be sure to read your blog post for a more in-depth understanding.
|
|
||
| handlePopoutOpen = (val) => { | ||
| this.setState({ popoutOpen: val }); | ||
| const handlePopoutOpen = (val) => { |
There was a problem hiding this comment.
Minor nit: from a code clarity perspective, I'm of the opinion that it's preferable to have functions declared with function, and reserve const for computed values. Not required to change, just my two-cents (it's highly likely that at some in this code, I was also defining functions with const).
b18050
force-pushed
the
refactor-class-to-functional
branch
from
7ecf415
to
7721787
Compare
Fix #147