Skip to content
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

Consistent disabled state for all components #19343

Open
2 tasks
LorenzHenk opened this issue Jan 22, 2020 · 9 comments
Open
2 tasks

Consistent disabled state for all components #19343

LorenzHenk opened this issue Jan 22, 2020 · 9 comments
Labels
breaking change design: material This is about Material Design, please involve a visual or UX designer in the process

Comments

@LorenzHenk
Copy link

LorenzHenk commented Jan 22, 2020

At the moment, the disabled color of the slider is theme.palette.grey[400]. Other inputs use theme.palette.action.disabled (also used e.g. here) and theme.palette.text.disabled.

The Slider should use theme.palette.action.disabled as well.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The Slider uses theme.palette.grey[400].

Expected Behavior 🤔

The Slider should use theme.palette.action.disabled.

Steps to Reproduce 🕹

Example showing that theme.palette.grey[400] is used: https://codesandbox.io/s/material-demo-qpzdk

image

Note: I've forced the color with !important for the screenshot above.

Actually, this demo shows another problem with the current style system - the custom disabled style is not applied, because the css specifity of the default disabled class is higher than the one of the custom class - is this wished? Should I create another issue for this problem?

image

image

Steps:

  1. Create a custom theme to see the difference between grey[400] and action.disabled
  2. set disabled to true

Context 🔦

We edit some values of the palette, which leads to a difference between grey[400] and actions.disabled. All disabled inputs have the same color, except for the Slider component.
image

Your Environment 🌎

Tech Version
Material-UI v4.8.3
@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Jan 22, 2020
@oliviertassinari oliviertassinari changed the title [Slider] disabled color should use theme action disabled color Consistent disabled state for all components Jan 22, 2020
@oliviertassinari
Copy link
Member

@LorenzHenk Interesting. The specification has a section about the disabled state:

A disabled state communicates when a component or element isn’t interactive, and should be deemphasized in a UI. Disabled states are displayed at 38% opacity of the enabled state.
Disabled states can also indicate they are inactive through color changes and reduced elevation.

https://material.io/design/interaction/states.html#disabled.

It seems that we can simplify things. What do you think of these changes?

diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index 25c676bdb..5cd94371b 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -208,7 +208,7 @@ export const styles = theme => ({
       backgroundColor: theme.palette.action.hover,
     },
     '&[aria-disabled="true"]': {
-      opacity: 0.5,
+      opacity: theme.palette.action.disabledOpacity,
       pointerEvents: 'none',
     },
     '&:active': {
diff --git a/packages/material-ui-lab/src/Rating/Rating.js b/packages/material-ui-lab/src/Rating/Rating.js
index 2089fd521..130f099b3 100644
--- a/packages/material-ui-lab/src/Rating/Rating.js
+++ b/packages/material-ui-lab/src/Rating/Rating.js
@@ -40,7 +40,7 @@ export const styles = theme => ({
     cursor: 'pointer',
     WebkitTapHighlightColor: 'transparent',
     '&$disabled': {
-      opacity: 0.5,
+      opacity: theme.palette.action.disabledOpacity,
       pointerEvents: 'none',
     },
     '&$focusVisible $iconActive': {
diff --git a/packages/material-ui/src/Chip/Chip.js b/packages/material-ui/src/Chip/Chip.js
index 70118bf44..bc014858e 100644
--- a/packages/material-ui/src/Chip/Chip.js
+++ b/packages/material-ui/src/Chip/Chip.js
@@ -38,7 +38,7 @@ export const styles = theme => {
       verticalAlign: 'middle',
       boxSizing: 'border-box',
       '&$disabled': {
-        opacity: 0.5,
+        opacity: theme.palette.action.disabledOpacity,
         pointerEvents: 'none',
       },
       '& $avatar': {
diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index f90c5fe9b..2725d0e64 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -142,7 +142,10 @@ export const styles = theme => ({
     '&$disabled': {
       pointerEvents: 'none',
       cursor: 'default',
-      color: theme.palette.grey[400],
+      color:
+        theme.palette.type === 'light'
+          ? lighten('#000000', 1 - theme.palette.action.disabledOpacity)
+          : darken('#ffffff', 1 - theme.palette.action.disabledOpacity),
     },
     '&$vertical': {
       width: 2,
diff --git a/packages/material-ui/src/Tab/Tab.js b/packages/material-ui/src/Tab/Tab.js
index 56424569b..7fd51ed93 100644
--- a/packages/material-ui/src/Tab/Tab.js
+++ b/packages/material-ui/src/Tab/Tab.js
@@ -44,7 +44,7 @@ export const styles = theme => ({
       opacity: 1,
     },
     '&$disabled': {
-      opacity: 0.5,
+      opacity: theme.palette.action.disabledOpacity,
     },
   },
   /* Styles applied to the root element if the parent [`Tabs`](/api/tabs/) has `textColor="primary"`. */
diff --git a/packages/material-ui/src/styles/createPalette.js b/packages/material-ui/src/styles/createPalette.js
index 32db973fe..3e13072b1 100644
--- a/packages/material-ui/src/styles/createPalette.js
+++ b/packages/material-ui/src/styles/createPalette.js
@@ -39,7 +39,8 @@ export const light = {
     // 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)',
+    disabled: 'rgba(0, 0, 0, 0.38)',
+    disabledOpacity: 0.38,
     // The background color of a disabled action.
     disabledBackground: 'rgba(0, 0, 0, 0.12)',
   },
@@ -63,7 +64,8 @@ export const dark = {
     hover: 'rgba(255, 255, 255, 0.1)',
     hoverOpacity: 0.1,
     selected: 'rgba(255, 255, 255, 0.2)',
-    disabled: 'rgba(255, 255, 255, 0.3)',
+    disabled: 'rgba(255, 255, 255, 0.38)',
+    disabledOpacity: 0.38,
     disabledBackground: 'rgba(255, 255, 255, 0.12)',
   },
 };

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Jan 22, 2020
@elmeerr
Copy link
Contributor

elmeerr commented Jan 22, 2020

@oliviertassinari how does the new changes regarding state anatomy impacts this issue? I might take a better look on those components

@oliviertassinari
Copy link
Member

@elmeerr I don't understand what you mean? I have tried to take it into account :).

@elmeerr
Copy link
Contributor

elmeerr commented Jan 22, 2020

@oliviertassinari oh, sorry! I just wanna know if it's possible to use the new properties from theme (like you suggested in green) to update more components (if it's needed)

@oliviertassinari
Copy link
Member

@elmeerr Right, I haven't looked into it. It's possible :)

@LorenzHenk
Copy link
Author

@oliviertassinari I've also found the opacity hardcoded in ExpansionPanelSummary and ListItem:

diff --git a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
index 25c676bdb..5cd94371b 100644
--- a/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
+++ b/packages/material-ui-lab/src/Autocomplete/Autocomplete.js
@@ -208,7 +208,7 @@ export const styles = theme => ({
       backgroundColor: theme.palette.action.hover,
     },
     '&[aria-disabled="true"]': {
-      opacity: 0.5,
+      opacity: theme.palette.action.disabledOpacity,
       pointerEvents: 'none',
     },
     '&:active': {
diff --git a/packages/material-ui-lab/src/Rating/Rating.js b/packages/material-ui-lab/src/Rating/Rating.js
index 2089fd521..130f099b3 100644
--- a/packages/material-ui-lab/src/Rating/Rating.js
+++ b/packages/material-ui-lab/src/Rating/Rating.js
@@ -40,7 +40,7 @@ export const styles = theme => ({
     cursor: 'pointer',
     WebkitTapHighlightColor: 'transparent',
     '&$disabled': {
-      opacity: 0.5,
+      opacity: theme.palette.action.disabledOpacity,
       pointerEvents: 'none',
     },
     '&$focusVisible $iconActive': {
diff --git a/packages/material-ui/src/Chip/Chip.js b/packages/material-ui/src/Chip/Chip.js
index 70118bf44..bc014858e 100644
--- a/packages/material-ui/src/Chip/Chip.js
+++ b/packages/material-ui/src/Chip/Chip.js
@@ -38,7 +38,7 @@ export const styles = theme => {
       verticalAlign: 'middle',
       boxSizing: 'border-box',
       '&$disabled': {
-        opacity: 0.5,
+        opacity: theme.palette.action.disabledOpacity,
         pointerEvents: 'none',
       },
       '& $avatar': {
diff --git a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
index 53db1fdc5..ea75713bb 100644
--- a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
+++ b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
@@ -28,7 +28,7 @@ export const styles = theme => {
         backgroundColor: theme.palette.grey[300],
       },
       '&$disabled': {
-        opacity: 0.38,
+        opacity: theme.palette.action.disabledOpacity,
       },
     },
     /* Pseudo-class applied to the root element, children wrapper element and `IconButton` component if `expanded={true}`. */
diff --git a/packages/material-ui/src/ListItem/ListItem.js b/packages/material-ui/src/ListItem/ListItem.js
index bb14d8aec..121c5ca73 100644
--- a/packages/material-ui/src/ListItem/ListItem.js
+++ b/packages/material-ui/src/ListItem/ListItem.js
@@ -29,7 +29,7 @@ export const styles = theme => ({
       backgroundColor: theme.palette.action.selected,
     },
     '&$disabled': {
-      opacity: 0.5,
+      opacity: theme.palette.action.disabledOpacity,
     },
   },
   /* Styles applied to the `container` element if `children` includes `ListItemSecondaryAction`. */
diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index f90c5fe9b..2725d0e64 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -142,7 +142,10 @@ export const styles = theme => ({
     '&$disabled': {
       pointerEvents: 'none',
       cursor: 'default',
-      color: theme.palette.grey[400],
+      color:
+        theme.palette.type === 'light'
+          ? lighten('#000000', 1 - theme.palette.action.disabledOpacity)
+          : darken('#ffffff', 1 - theme.palette.action.disabledOpacity),
     },
     '&$vertical': {
       width: 2,
diff --git a/packages/material-ui/src/Tab/Tab.js b/packages/material-ui/src/Tab/Tab.js
index e5042d6a1..f4c5931b0 100644
--- a/packages/material-ui/src/Tab/Tab.js
+++ b/packages/material-ui/src/Tab/Tab.js
@@ -43,7 +43,7 @@ export const styles = theme => ({
       opacity: 1,
     },
     '&$disabled': {
-      opacity: 0.5,
+      opacity: theme.palette.action.disabledOpacity,
     },
   },
   /* Styles applied to the root element if the parent [`Tabs`](/api/tabs/) has `textColor="primary"`. */
diff --git a/packages/material-ui/src/styles/createPalette.js b/packages/material-ui/src/styles/createPalette.js
index cbb66038f..8075d6044 100644
--- a/packages/material-ui/src/styles/createPalette.js
+++ b/packages/material-ui/src/styles/createPalette.js
@@ -40,7 +40,8 @@ export const light = {
     selected: 'rgba(0, 0, 0, 0.08)',
     selectedOpacity: 0.08,
     // The color of a disabled action.
-    disabled: 'rgba(0, 0, 0, 0.26)',
+    disabled: 'rgba(0, 0, 0, 0.38)',
+    disabledOpacity: 0.38,
     // The background color of a disabled action.
     disabledBackground: 'rgba(0, 0, 0, 0.12)',
   },
@@ -65,7 +66,8 @@ export const dark = {
     hoverOpacity: 0.08,
     selected: 'rgba(255, 255, 255, 0.16)',
     selectedOpacity: 0.16,
-    disabled: 'rgba(255, 255, 255, 0.3)',
+    disabled: 'rgba(255, 255, 255, 0.38)',
+    disabledOpacity: 0.38,
     disabledBackground: 'rgba(255, 255, 255, 0.12)',
   },
 };

@TommyJackson85
Copy link
Contributor

Can I do anything to help? As a first issue?

@oliviertassinari
Copy link
Member

@LorenzHenk Sorry for the delay. Do you want to work on the problem? I have updated #10870 to track all the progress on this epic (umbrella issue).

@LorenzHenk
Copy link
Author

Sure!

@oliviertassinari oliviertassinari added this to the v5 milestone Sep 18, 2020
@oliviertassinari oliviertassinari added breaking change and removed good first issue Great for first contributions. Enable to learn the contribution process. labels Sep 18, 2020
@oliviertassinari oliviertassinari modified the milestones: v5-beta, v5 Jul 1, 2021
@mnajdova mnajdova modified the milestones: v5, v6 Aug 16, 2021
@DiegoAndai DiegoAndai removed the v6.x label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
Status: Stalled
Development

Successfully merging a pull request may close this issue.

6 participants