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

feat(menu): update menu and menu items styles #1166

Merged
merged 5 commits into from Oct 18, 2023
Merged

Conversation

pedrobonamin
Copy link
Contributor

@pedrobonamin pedrobonamin commented Oct 17, 2023

Updates menu and menu item styles to match facelift.
Refactors menu item to support new props:

  • subText
  • badgeText

https://sanity-ui-storybook-git-edx-484-menu.sanity.build/?path=/story/components-menu--menu-items-variants&globals=theme:light

Examples:

  • Left - Menu items enabled.
  • Right - Menu items disabled.

Light mode

Screenshot 2023-10-17 at 13 16 10

Dark mode

Screenshot 2023-10-17 at 13 16 16

@vercel
Copy link

vercel bot commented Oct 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sanity-ui-workshop ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 18, 2023 0:04am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
sanity-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Oct 18, 2023 0:04am

Copy link
Contributor

Choose a reason for hiding this comment

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

question: what's the rationale for applying hover styles to both active and selected states here? These changes mean that it's not possible to disambiguate between the 'selected' menu item and the one that's being currently hovered.

To better illustrate, try visit this example and click any menu item and then press the up or down arrows – you'll be able to see the item that is 'selected' whilst having a hover state on the previously clicked button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @robinpyon In the figma file the selected and hover states are the same that's the reason they are repeated.
With the redesign, the tint used for the selected state background is 50 so we don't have a way to set a lighter color in hover like in the example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying @pedrobonamin! Possibly something we can iterate on in future, I do think there's value in differentiating between those states.

@pedrobonamin pedrobonamin merged commit 7269eb9 into alpha Oct 18, 2023
10 checks passed
@pedrobonamin pedrobonamin deleted the edx-484-menu branch October 18, 2023 16:21
mariuslundgard pushed a commit that referenced this pull request Nov 2, 2023
* feat(menu): add menu stories and update menu shadows

* feat(menu-item): add menu item stories, update selectable styles
mariuslundgard pushed a commit that referenced this pull request Nov 7, 2023
* feat(menu): add menu stories and update menu shadows

* feat(menu-item): add menu item stories, update selectable styles
mariuslundgard pushed a commit that referenced this pull request Dec 7, 2023
* feat(menu): add menu stories and update menu shadows

* feat(menu-item): add menu item stories, update selectable styles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants