-
-
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
[docs] Improve settings toggle button styling #23754
Conversation
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.
It looks better. What if we added the support for a color prop on the component itself?
I wondered about that. I'll open an issue for it. |
dfd553d
to
c8d9a42
Compare
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.
Oops, yes. When I was forcing it with |
@@ -37,6 +37,9 @@ const styles = (theme) => ({ | |||
'&$toggleButtonSelected': { | |||
color: `${theme.palette.primary.main}`, | |||
backgroundColor: `${alpha(theme.palette.primary.main, theme.palette.action.selectedOpacity)}`, | |||
'&:hover': { | |||
backgroundColor: `${alpha(theme.palette.primary.main, 0.12)}`, |
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.
See https://github.com/mui-org/material-ui/blob/6f56be2a07185be06984476aed9f518cbbd6b87d/packages/material-ui-lab/src/TreeItem/TreeItem.js#L50-L58 for a good approach, I think that we miss the hover none and the design tokens for the alpha value.
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.
theme.palette.action.selectedOpacity + theme.palette.action.hoverOpacity
Ah, I didn't think of summing them. 👍
By comparison, an active list item doesn't change shade when hovered.
Perhaps there should be a distinction between a toggle button that can be deselected (changes shade when hovered), and one that can't (doesn't change shade when hovered)?
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.
By comparison, an active list item doesn't change shade when hovered.
It seems to do in my case: https://next.material-ui.com/components/lists/#selected-listitem. We marked it as handled in #10870.
Perhaps there should be a distinction between a toggle button that can be deselected (changes shade when hovered), and one that can't (doesn't change shade when hovered)?
Why not. This new design makes me think about https://ant.design/components/radio/#Radio/Radio.Button
I'd forgotten about that. I think I checked on https://material-ui.com In this example the active item doesn't change color on hover (i.e. doesn't lighten when the mouse moves off after selection): (Taken from https://material.io/design/interaction/states.html#activated, Behavior)
Except that, in the case of Ant, the active item does change on hover. BTW it seems the toggle button spec has ben updated to be higher contrast the Toggle Button spec: https://material.io/components/buttons#toggle-button, so we can move some of these changes into ToggleButton. I'm going to commit them here first though to fix the Settings panel in the short term. I'll update them when ToggleButton is updated. There's also some precedence in the spec for a colored active state, see update in: #23755 |
Material Design mentions "can", so it might be up to the designer. |
Higher contrast, and use primary color for selected state: