-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
MenuItem Fix hoverColor muiTheme override #4957
Conversation
…he menuItem.hoverColor in muiTheme.
@@ -297,6 +297,7 @@ class MenuItem extends Component { | |||
ref="listItem" | |||
rightIcon={rightIconElement} | |||
style={mergedRootStyles} | |||
hoverColor={this.context.muiTheme.menuItem.hoverColor} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other properties are alphabetically sorted. Might be a coincidence for all I know.
@@ -28,7 +28,7 @@ function getStyles(props, context, state) { | |||
const {listItem} = muiTheme; | |||
|
|||
const textColor = muiTheme.baseTheme.palette.textColor; | |||
const hoverColor = fade(textColor, 0.1); | |||
const hoverColor = props.hoverColor || fade(context.muiTheme.baseTheme.palette.textColor, 0.1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keeping the textColor
?
53b828e
to
ea2538e
Compare
Looks like there is another PR opened for the same issue #5000. It would be great to join forces to address the issue. |
@@ -291,6 +291,7 @@ class MenuItem extends Component { | |||
<ListItem | |||
{...other} | |||
disabled={disabled} | |||
hoverColor={this.context.muiTheme.menuItem.hoverColor} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I get it now muiTheme.menuItem.hoverColor
is unused as we are speaking 😨 .
Any update on this? I need this fix for my current project. I'd be happy to help if I can. |
@oliviertassinari I consider it a fix because it's a property that can be set when customizing the theme, but setting that property has no effect because the component itself hard-codes it based on the text color. Is that not a bug? I need to be able to set the hover color independently of the text color. I understand #5000 is also addressing this issue. I was just wondering if I could be any help getting one of them in. |
@garrettn Oh right, thanks for the context. |
I opened a new PR at #5502. |
Fix the MenuItem hoverColor override when specified. muiTheme.menuItem.hoverColor had no effect. Passes hoverColor to ListItems if muiTheme.menuItem.hoverColor specified. Fix closes #4227