Open SebastianSchmidt opened 6 years ago
@oliviertassinari @mbrookes What do you think about making the hover effects of Material-UI consistent? I think that would improve the usability of the components. I'd like to take care of it, but I'd like to know your opinion beforehand.
I have no fundamental objection, but I'd like to see how each one looks, even if just a mockup, or ideally a link to another implementation. StepButton seems likely to be the most problematic (visiually).
I have no fundamental objection
Me neither. I do agree that consistency is important. I believe it's dependent on #9526.
It's an area of investigation: It might make sense to move the logic to the ButtonBase component.
We could have a property to toggle to disable the hover behavior. We could also rely on currentColor
and opacity
to get the correct value. It would be more flexible than hardcoding the background color. Well no, forget it. I think that it will have the opposite effect, making overrides harder.
I have also realized that we are using fade(0.12) while MWC is using 0.04. Should we follow them? Should we use a theme variable for consistency?
Do we know if MCW fade() algo is constant with ours?
Should we use a theme variable for consistency?
Might be no harm if it's going to be used in multiple components.
I am in favour of continuing to use fade(0.12)
because the contrast of the hover effect to the background is too weak when fade(0.04)
is used. (Edit: Another alternative would be fade(0.08)
.)
fade(0.12) |
face(0.08) |
fade(0.04) |
---|---|---|
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
![]() |
@SebastianSchmidt I can understand the issue. I have been wondering too. At least, It's not an issue on white backgrounds.
I have noticed that there is already a theme variable for hover effects. However, this value is currently only used for the ListItem component; and contains another alpha value: 0.08. MDC uses 0.04 for the hover effect for the list items - consistent to the buttons. I think we should also use 0.12 (or 0.04) consistently for the list items.
@oliviertassinari @mbrookes What do you think: Should the alpha value be moved to a separate variable or should the existing variable be changed so that it contains only the alpha value (= breaking change)?
// The colors used to style the action elements.
action: {
// The color of an active action like an icon button.
active: 'rgba(0, 0, 0, 0.54)',
// The color of an hovered action.
hover: 'rgba(0, 0, 0, 0.08)',
// The color of a selected action.
selected: 'rgba(0, 0, 0, 0.14)',
// The color of a disabled action.
disabled: 'rgba(0, 0, 0, 0.26)',
// The background color of a disabled action.
disabledBackground: 'rgba(0, 0, 0, 0.12)',
},
// ...
action: {
active: common.white,
hover: 'rgba(255, 255, 255, 0.1)',
selected: 'rgba(255, 255, 255, 0.2)',
disabled: 'rgba(255, 255, 255, 0.3)',
disabledBackground: 'rgba(255, 255, 255, 0.12)',
},
and contains another alpha value: 0.08
This sounds like an inconsistency we need to fix, good catch 👍.
What do you think: Should the alpha value be moved to a separate variable or should the existing variable be changed so that it contains only the alpha value (= breaking change)?
I think that it would be best to use a separate alpha variable.
All right, then the next step is to add the new theme variable and use it in the components Button, IconButton and ListItem. I'll take care of it!
@elmeerr Interesting, looking more closely at the hover opacity. This looks more or less OK but we could make it a bit lighter:
rgba(0, 0, 0, 0.07)
=> hsl(0, 0%, 93%)rgba(0,0,0,.04)
=> hsl(0, 0%, 96%)#eeeeee
=> hsl(0, 0%, 93%)#f1f3f4
=> hsl(200, 12%, 95%)#eeeeee
=> hsl(0, 0%, 93%)rgba(0,0,0,.04)
=> hsl(0, 0%, 96%)However, the selected color seems off, it should be colored (primary/secondary color?):
rgba(0, 0, 0, 0.04)
rgba(161,194,250,.16)
#e8f0fe
#e8f0fe
rgba(98,0,238,.04)
Do you want to work on the problem?
@oliviertassinari indeed the selected is off (but it's applying the "correct" alpha) as far as I can tell, it should be the primary color + alpha .04
I can double check. I'll create a PR when I finish
To be clear, I don't think that MDC shouldn't be given a higher weight than any of the other google's products. I think that http://material.io/ should be our primary reference.
I am very excited about the direction this is going as it seems to more closely align with the Material spec.
One practical challenge I am running into is that I would like the ability to set the opacity of a given state (e.g. hover) based on the underlying surface color. In particular on darker surfaces, higher opacities are required in order to provide adequate visual contrast. I believe this is consistent with the implications on material.io (https://material.io/design/interaction/states.html#anatomy).
If I understand correctly, currently there is only a single theme value for controlling opacity for a given state (e.g. palette.action.hoverOpacity
). Would it be possible to provide overrides at the palette color level? Or otherwise provide a function that could be given the currentColor and return the desired opacity?
Material Design provides a page: https://material.io/design/interaction/states.html#usage on how to style the different states. States are visual representations used to communicate the status of a component or an interactive element.
The components to handle:
Related issues:
5186: list states
19343: disabled state
17493: button states