Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nav menu - redesign implementation #12125

Merged
merged 138 commits into from
Feb 23, 2024
Merged

Nav menu - redesign implementation #12125

merged 138 commits into from
Feb 23, 2024

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Feb 6, 2024

Description

Implements the newly redesigned navigation menu for both desktop and mobile, including changes to the information architecture

  • Figma design
  • Updates the information architecture inside the useNav hook with new menu item structure, updated labels and addition of item descriptions/subtext.

Desktop

  • Uses Radix-UI library for NavigationMenu component
  • Includes keyboard navigation/a11y

Mobile

  • Uses Chakra-UI Accordion component

Preview link

Related issue

TODO

  • Import translated intl strings
  • Improve animations

clean up icon/button components, including HamburgerButton and CloseButton
close mobile menu by clicking outside of drawer
scrolls container to end when third level opens on tablet size devices
removes stray ms={2} from Static layout content
@wackerow wackerow marked this pull request as ready for review February 18, 2024 05:21
@wackerow
Copy link
Member Author

Flipped this to "ready for review" but please do not merge until translated strings are imported, which is the last blocking todo.

@konopkja
Copy link
Contributor

Screenshot 2024-02-19 at 16 57 27

we have a little bit of inconsistency with use cases, can we change Desci and Refi to be same format as with main use cases? (acronym first, full name second)

@nloureiro
Copy link
Contributor

[bug] @wackerow, I think this spacing is too big. There is an extra margin that leaves the navigation too far out for the glyph

Screen Shot 2024-02-19 12 07 43 AM

@nloureiro
Copy link
Contributor

nloureiro commented Feb 19, 2024

@wackerow, there are two bugs in the menu's interactivity. We can chat about it if it needs more details.

  1. I think it would be great if we could open the menu by clicking and by hovering with delay, but it creates an issue:
  • Accidental closing of the menu when clicking >>> I'm reading the menu and hover one option > it opens with the hover delay > and because it delays a little bit, I click too > accidentally closes the menu.

solution: add the condition: if open && hover { disable the close option }

  1. The gap between the nav button and the menu causes it to close without the intention to, but visually, everything looks great, the button size included.
  • if the user takes a little bit, more than the delay, to go from the header to the menu, it closes the menu

Solution????: make the button go all the way to the bottom part of the header, with an inside tag that is the size of the actual button to create balance visually, but the interaction keeps the menu open (not sure it makes sense, open to discuss other solutions )

Screen Shot 2024-02-19 01 52 54 PM

src/components/Nav/Menu/index.tsx Outdated Show resolved Hide resolved
src/components/Nav/Menu/index.tsx Show resolved Hide resolved
src/components/Nav/Menu/index.tsx Show resolved Hide resolved
@pettinarip
Copy link
Member

@wackerow, there are two bugs in the menu's interactivity. We can chat about it if it needs more details.

  1. I think it would be great if we could open the menu by clicking and by hovering with delay, but it creates an issue:
  • Accidental closing of the menu when clicking >>> I'm reading the menu and hover one option > it opens with the hover delay > and because it delays a little bit, I click too > accidentally closes the menu.

solution: add the condition: if open && hover { disable the close option }

  1. The gap between the nav button and the menu causes it to close without the intention to, but visually, everything looks great, the button size included.
  • if the user takes a little bit, more than the delay, to go from the header to the menu, it closes the menu

Solution????: make the button go all the way to the bottom part of the header, with an inside tag that is the size of the actual button to create balance visually, but the interaction keeps the menu open (not sure it makes sense, open to discuss other solutions )

@wackerow
For 1, it seems like a known issue radix-ui/primitives#2326 with a potential workaround here radix-ui/primitives#1630 (comment)

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🚀 🚀 🚀

Locales: am, ca, fi, fil, gl, he, hi, hr, ka, km, kn, ko, ml, ml, mr, ms, nb, nl, pcm, pt, pt-br, ro, ru, se, sk, sl, sr, sw, ta, th, tr, uk, ur, vi
@corwintines corwintines self-assigned this Feb 22, 2024
Copy link
Member

@corwintines corwintines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a couple of comments on some spelling, but this looks good otherwise to me! Nice work!

Co-authored-by: Corwin Smith <cssmittys@gmail.com>
@corwintines corwintines merged commit e7bc5de into dev Feb 23, 2024
9 of 10 checks passed
@corwintines corwintines deleted the nav-menu-radix-ui branch February 23, 2024 19:30
This was referenced Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Changes related to project dependencies tooling 🔧 Changes related to tooling of the project translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants