Navigation Menu

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(overflow-menu): use correct icon color for danger items #3147

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Jun 21, 2019

Closes #2971

This PR matches SVG icon color with text color in danger/delete variant overflow menu items on hover (similar to icons in buttons)

Changelog

Changed

  • isDelete overflow menu item SVG icon hover color

Testing / Reviewing

Add an icon to an overflow menu item with isDelete={true} to test. as far as I know this isn't a pattern that we expose on our docs so it isn't viewable on Netlify

you can test this by modifying one of the overflow menu items in storybook to

<OverflowMenuItem
  {...overflowMenuItemProps}
  itemText={
    <>
      <Delete16 /> Danger option
    </>
  }
  hasDivider
  isDelete
/>

@emyarod emyarod added this to the v10.4 milestone Jun 21, 2019
@emyarod emyarod added this to PR: needs review 🕵️ in Carbon support via automation Jun 21, 2019
@netlify
Copy link

netlify bot commented Jun 21, 2019

Deploy preview for the-carbon-components ready!

Built with commit de02a5b

https://deploy-preview-3147--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jun 21, 2019

Deploy preview for carbon-components-react ready!

Built with commit de02a5b

https://deploy-preview-3147--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Jun 21, 2019

Deploy preview for carbon-elements ready!

Built with commit de02a5b

https://deploy-preview-3147--carbon-elements.netlify.com

Carbon support automation moved this from PR: needs review 🕵️ to PR: ready to merge 🔜 Jun 21, 2019
@joshblack
Copy link
Contributor

@emyarod mind checking in the built files? I think it'll just be yarn docs in packages/components

@asudoh
Copy link
Contributor

asudoh commented Jun 22, 2019

@joshblack Seems that this is style-only change with a few lines...?

@joshblack
Copy link
Contributor

@asudoh since it uses a token, the SassDoc for components needs to be updated since it captures token usage.

@asudoh
Copy link
Contributor

asudoh commented Jun 24, 2019

@joshblack Sounds that this topic requires update to contributor guide. Would you please do? Thanks!

@emyarod emyarod force-pushed the 2971-overflow-menu-danger-item-icon-color branch from 37f4760 to 7109ad6 Compare June 24, 2019 13:06
@emyarod
Copy link
Member Author

emyarod commented Jun 24, 2019

updated the sassdoc but the error still persists, does the script need to be tweaked?

@jnm2377
Copy link
Contributor

jnm2377 commented Jun 24, 2019

Try running yarn build at the top-level. That's what I had to do for one of my PR's that was failing with the same error.

@emyarod emyarod force-pushed the 2971-overflow-menu-danger-item-icon-color branch from 7109ad6 to dc5a61a Compare June 24, 2019 21:31
@tw15egan tw15egan merged commit f06f38f into carbon-design-system:master Jun 26, 2019
Carbon support automation moved this from PR: ready to merge 🔜 to Issue/PR: Closed 🙌 Jun 26, 2019
@emyarod emyarod deleted the 2971-overflow-menu-danger-item-icon-color branch June 27, 2019 13:34
@emyarod emyarod mentioned this pull request Jul 15, 2019
41 tasks
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.

isDelete=true OverflowMenuItem Icon should be white onmouseover
5 participants