[theme] Use a single theme variable for the hover effects of Button, IconButton and ListItem #10952
Conversation
src/styles/createPalette.js
Outdated
| @@ -32,7 +32,7 @@ export const light = { | |||
| // 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)', | |||
| hover: 'rgba(0, 0, 0, 0.12)', | |||
There was a problem hiding this comment.
Maybe I've misunderstood, as I've only looked at this superficially, but since @oliviertassinari was concerned that 0.12 is too opaque, and you in turn that 0.04 is too transparent, isn't 0.08 the perfect compromise?
There was a problem hiding this comment.
@mbrookes @oliviertassinari Yes, 0.08 looks good! I have added this value to the table with the examples and modified the pull request.
SebastianSchmidt
changed the title
|
The hover effects of Button and IconButton now use the same transparency as ListItem. |
src/styles/colorManipulator.test.js
Outdated
| @@ -187,6 +187,17 @@ describe('utils/colorManipulator', () => { | |||
| assert.strictEqual(fade('hsla(0, 100%, 50%, 0.2)', 0.5), 'hsla(0, 100%, 50%, 0.5)'); | |||
| }); | |||
|
|
|||
| it('sets the transparency of a color to the transparency of the color provided', () => { | |||
| assert.strictEqual( | |||
| fade('rgba(255, 0, 0, 0.2)', 'rgba(1, 2, 3, 0.5)'), | |||
There was a problem hiding this comment.
Do we want to support this API? This looks counter-intuitive to me.
There was a problem hiding this comment.
My first reaction was "yuck", so I"m glad it wasn't just me! 😄 (Sorry @SebastianSchmidt!)
There was a problem hiding this comment.
Do you think the approach of reusing the existing theme variable in this way is basically correct? Then I would move the logic for extracting and applying transparency to its own function.
There was a problem hiding this comment.
I was thinking of adding a theme.palette.action.hoverOpacity = 0.08 property or something like that.
There was a problem hiding this comment.
@oliviertassinari In my opinion, adding a new variable causes the following problem:
The transparency of hover effects is configured in two variables and must be kept consistent with each other. If a user changes only one of the two variables, the hover effects of the components are inconsistent. The user does not necessarily notice this error directly; and later the user wonders why the components of his user interface are inconsistent with each other.
To avoid this error, I would suggest to use only one variable and extract the required information (the degree of transparency) via a help function. This simplifies the configuration of the theme for the user.
There was a problem hiding this comment.
The transparency of hover effects is configured in two variables and must be kept consistent with each other.
@SebastianSchmidt It's a tradeoff, I can live with it. But why not addressing the duplication issue 👍 . The helper function can do it. Should we keep the intermediary result (hoverOpacity) in the theme or compute it everytime?
I have realized that the current fade value (0.04) is close to imperceptible with the dark theme.
…IconButton and ListItem
oliviertassinari
added
core
design: material
In #10870 it was decided that all hover effects should be created based on a theme variable to ensure a consistent hover effect in all components. Instead of creating a new theme variable I used the existing theme variable
theme.palette.action.hoverto keep customizing the theme simple.For this purpose I have enhanced the existing function
fade. This function sets the absolute transparency of a color. So far, the new transparency could be indicated by a number. Now you can also specify a CSS color instead, whose transparency is used. In this way, the transparency from the existing theme variabletheme.palette.action.hovercan be reused. The advantage of this approach is that all hover effects can be customized using a single theme variable.