Fix IconButton styles for hover, focus, and pressed.#4498
Fix IconButton styles for hover, focus, and pressed.#4498jordandrako merged 6 commits intomicrosoft:masterfrom
Conversation
| rootHovered: { | ||
| color: theme.palette.themeDarker | ||
| color: theme.palette.themeDarker, | ||
| backgroundColor: theme.palette.neutralLighter, |
There was a problem hiding this comment.
Are there semantic color slots we can use for these?
There was a problem hiding this comment.
Yeah.
semanticColors.buttonBackground = palette.neutralLighter (the color Ben requested for Hovered state)
semanticColors.buttonBackgroundHovered = palette.neutralLight (the color Ben requested for Pressed state)
There was a problem hiding this comment.
Awesome--if that's the case, I'd think we should use those instead so IconButton can pick up themes moving forward. @betrue-final-final @phkuo for FYI
There was a problem hiding this comment.
Alright, I'll change all those over. Is there a semanticColor for palette.themePrimary or leave those alone in this case?
There was a problem hiding this comment.
Totally fair--duh. I must have missed the background portion. There doesn't seem to a perfect match--menuIcon seems like the next closest, but it feels wrong to apply "menu" to something that isn't in a menu.
@phkuo -- is this a case where we'd want to consider a new slot, or is it acceptable to stretch the usage of an existing slot, like using menuIcon for the foreground of an icon button?
There was a problem hiding this comment.
There are three possible ones I'd use: menuIcon, menuHeader, or link.
@mikewheaton Does palette.themePrimary need to be replaced in scenarios like this? Doesn't it get its value from the theme HOC too?
There was a problem hiding this comment.
@jordandrako both palette and semantic colors theme. If uncustomized, semantic colors will pick up an appropriate palette color.
Don't try to "stretch" the meaning of a semantic slot too far, we shouldn't use menu slots for buttons. We do need new color slots for "text buttons", buttons that aren't outlined.
There was a problem hiding this comment.
Alright, then I think we should leave the themePrimary uses alone since they're applied to the IconButton's foreground color. @Jahnp cool?
There was a problem hiding this comment.
Ok, as long as having a mix of semantic & palette colors is fine, that works for me.
|
MenuIcon seems like a shortsighted color name. Should it be "commandIcon"? |
|
@MLoughry just a heads up |
5 participants
Pull request checklist
$ npm run changeDescription of changes
Fix IconButton styles when hovered, and pressed.
Normal:
Hovered:
Pressed:
Focus:
Disabled:
