Shadcn migration - desktop nav menu & search modal fixes #13647
Conversation
github-actions
bot
added
the
dependencies 📦
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Initial observations:
Animations are a bit more jumpy, and flashy... this could bother people with certain sensitivities. Recording shows production first, then PR:
Screen.Recording.2024-08-15.at.17.47.07.mov
Prod:
PR:
- Gap is now present between the selected menu item and the next column, when before were connected
- Search previously collapsed to the magnifying glass icon, while now it collapses to a box with just a keyboard command
@wackerow thanks for the early feedback. Good catch, didn't notice that while doing the transition, will take a look and see what Im missing.
Correct, I still need to implement this case.
Good observation, I must have missed something here. Thanks |
|
Awesome @pettinarip ! Screen.Recording.2024-08-20.at.10.34.14.mov
Tiny thing I'm noting, which I'll let @nloureiro or @konopkja weigh in on, where the background of the sub-section fades in, the parent/trigger background overlaps a little which didn't happen before. It's quick and subtle, just noting Second thing, just noting we lost our "hover" styling for the magnifying glass / search component. |
|
@wackerow fixed the extra spacing on the open menus and fixed the hover colors for the search icon. Good eyes, thanks for the level of detail I was missing. |
There was a problem hiding this comment.
This looks great @pettinarip! Left a couple small comments, but don't see those as blockers to this PR (can be addresses separately).
Pulling in!
| @@ -20,20 +22,12 @@ const DesktopNavMenu = ({ toggleColorMode }: DesktopNavMenuProps) => { | |||
| const { locale } = useRouter() | |||
| const languagePickerRef = useRef<HTMLButtonElement>(null) | |||
|
|
|||
| const ThemeIcon = useColorModeValue(<MdBrightness2 />, <MdWbSunny />) | |||
| const ThemeIcon = useColorModeValue(MdBrightness2, MdWbSunny) | |||
There was a problem hiding this comment.
Not at all against this, but just noting an alternative way this can be done as we migrate to TW:
<MdBrightness2 className="dark:hidden" />
<MdWbSunny className="not-[dark]:hidden" />Can see where both approaches could come in handy, not sure I have a strong preference or desire to standardize it.
There was a problem hiding this comment.
Great idea! I haven't thought about that option, I like the no-js dependency. I'll test it out and let you know.
Description
Styles the desktop nav menu using Tailwind & refactors the search modal logic.