feat: add theme param url behaviour with react-router #69
Conversation
hunxjunedo
requested review from
a team and
tjtanjin
and removed request for
tjtanjin
There was a problem hiding this comment.
The idea is great, it brings good quality of life improvements for users. That said, I notice several double assertions (e.g. theme as unknown as boolean). The focusedTheme state seems to imply that it will hold the value of the theme being focused on, but it turns out to be a boolean. Perhaps, consider having focusedTheme be of type string or null instead of a boolean, which would remove the need for double assertions. Checks would then be performed based no whether there is a theme focused.
In addition, the ThemeModal has been moved up to the Themes page. Since its rendering is now conditional based on focusedTheme, we also no longer need the isOpen prop (which takes its value from focusedTheme) passed into ThemeModal.
|
Yes, I left some refactoring that had to be done, this is not complete yet 👍 |
Description
makes a theme focused directly thru the url with the help of react-router and searchParams
Closes #66