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

fix(menu): update styles #15236

Conversation

alisonjoseph
Copy link
Member

@alisonjoseph alisonjoseph commented Nov 21, 2023

Closes #14676

Changelog

New

  • add title to MenuItem
  • add max-height value to Menu

Changed

  • update spacing between menu and menu buttons to 0px for MenuButton and ComboButton
  • update tokens for Menu $text-primary to $text-secondary, and explicityly set svgs inside to $icon-secondary,

Testing / Reviewing

Check Menu, MenuButton and ComboButton that they display correctly with updated tokens and that long text that overflows shows title on hover.

Review scrollbar on MenuButton default story

Copy link

netlify bot commented Nov 21, 2023

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 53580e6
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6568d4b6df064f00084980e4
😎 Deploy Preview https://deploy-preview-15236--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@thyhmdo
Copy link
Member

thyhmdo commented Nov 27, 2023

@alisonjoseph this looks good! The only one thing is we still maintain the text-primary on hover and focus + hover states

Here is the reference https://carbondesignsystem.com/components/menu/style#interactive-state-color

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Nov 27, 2023

@thyhmdo just updated the hover color to the primary tokens. Do you know what the max-height value should be?

@alisonjoseph alisonjoseph requested review from thyhmdo and removed request for thyhmdo November 28, 2023 18:20
@alisonjoseph alisonjoseph marked this pull request as ready for review November 28, 2023 19:46
@alisonjoseph alisonjoseph requested a review from a team as a code owner November 28, 2023 19:46
@andreancardona
Copy link
Contributor

andreancardona commented Nov 30, 2023

The only weird thing I see is here - seems the dropdown content doesn't stick to the menubutton - not sure if we care to address this or not 👀 @alisonjoseph

Screen.Recording.2023-11-30.at.10.11.48.mov

@alisonjoseph
Copy link
Member Author

@andreancardona I see that too, unrelated to anything in this PR though

Copy link
Member

@thyhmdo thyhmdo left a comment

Choose a reason for hiding this comment

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

Looks great!

@github-actions github-actions bot added this pull request to the merge queue Dec 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 4, 2023
@tay1orjones tay1orjones merged commit 593c22d into carbon-design-system:main Dec 4, 2023
21 checks passed
danoro96 pushed a commit to danoro96/carbon that referenced this pull request Jan 18, 2024
* fix(menu): update spacing and add title to menuitem

* fix(menu): add max-height and scroll to menu

* fix: set hover color menu to primary

* chore: remove max-height value and revert storybook changes

* chore: storybook example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Menu and Menu buttons: dev/style updates
4 participants