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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Stepper] Cannot customize StepIcon via rule name for style override #26830

Closed
2 tasks done
jlin5 opened this issue Jun 18, 2021 · 10 comments 路 Fixed by #26953
Closed
2 tasks done

[Stepper] Cannot customize StepIcon via rule name for style override #26830

jlin5 opened this issue Jun 18, 2021 · 10 comments 路 Fixed by #26953
Labels
bug 馃悰 Something doesn't work component: stepper This is the name of the generic UI component, not the React module!

Comments

@jlin5
Copy link

jlin5 commented Jun 18, 2021

  • 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 StepIcon's completed property does not have the override style applied.

Expected Behavior 馃

The StepIcon's completed property should have the override style applied. For example, the icon color should be green on completed={true} if the theme override has color: "green".

Steps to Reproduce 馃暪

https://codesandbox.io/s/epic-hawking-ijkyj?file=/src/App.tsx

    MuiStepIcon: {
      styleOverrides: {
        completed: {
          color: "green",
        },
      },
    },

Using the rule name of the component's styleOverrides property in a custom theme is not applied.

@jlin5 jlin5 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 18, 2021
@jlin5 jlin5 changed the title [StepIcon] Cannot customize StepIcon via style override [StepIcon] Cannot customize StepIcon via rule name for style override Jun 18, 2021
@michal-perlakowski
Copy link
Contributor

michal-perlakowski commented Jun 21, 2021

Try this instead:

import { stepIconClasses } from "@material-ui/core/StepIcon";

styleOverrides: {
  root: {
    [`&.${stepIconClasses.completed}`]: {
      color: "green"
    }
  }
}

https://codesandbox.io/s/pensive-thompson-gc07g?file=/src/App.tsx

@jlin5
Copy link
Author

jlin5 commented Jun 21, 2021

Is it encouraged to use the classNames instead of the rule name provided? I know that the text rule name works so I would assume completed would work as well.

@siriwatknp siriwatknp added bug 馃悰 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 24, 2021
@siriwatknp

This comment has been minimized.

@siriwatknp

This comment has been minimized.

@michal-perlakowski
Copy link
Contributor

michal-perlakowski commented Jun 24, 2021

@siriwatknp That fix doesn't seem to be working. Based on the Dialog implementation, I think it should be like this:

diff --git a/packages/material-ui/src/StepIcon/StepIcon.js b/packages/material-ui/src/StepIcon/StepIcon.js
--- a/packages/material-ui/src/StepIcon/StepIcon.js	(revision 6ef3bbe657c9447d649ef044d45e9bdec2c5d86d)
+++ b/packages/material-ui/src/StepIcon/StepIcon.js	(date 1624528599570)
@@ -23,17 +23,20 @@
 const StepIconRoot = styled(SvgIcon, {
   name: 'MuiStepIcon',
   slot: 'Root',
-  overridesResolver: (props, styles) => styles.root,
-})(({ theme }) => ({
+  overridesResolver: ({ styleProps }, styles) => ([
+    styles.root,
+    (styleProps.completed && styles.completed),
+  ]),
+})(({ theme, styleProps }) => ({
   /* Styles applied to the root element. */
   display: 'block',
   transition: theme.transitions.create('color', {
     duration: theme.transitions.duration.shortest,
   }),
   color: theme.palette.text.disabled,
-  [`&.${stepIconClasses.completed}`]: {
+  ...(styleProps.completed && {
     color: theme.palette.primary.main,
-  },
+  }),
   [`&.${stepIconClasses.active}`]: {
     color: theme.palette.primary.main,
   },

@siriwatknp

This comment has been minimized.

@oliviertassinari oliviertassinari changed the title [StepIcon] Cannot customize StepIcon via rule name for style override [Stepper] Cannot customize StepIcon via rule name for style override Jun 24, 2021
@oliviertassinari oliviertassinari added the component: stepper This is the name of the generic UI component, not the React module! label Jun 24, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 24, 2021

This seems correct #26830 (comment), but none of the proposed diff.

I think that the completed state should be treated as a pseudo-classes, like the active or error states. The styles are written for it (bump of specificity). This is important to allow developers to customize the baseline state, without overriding the specialized ones.

diff --git a/packages/material-ui-unstyled/src/generateUtilityClass/generateUtilityClass.ts b/packages/material-ui-unstyled/src/generateUtilityClass/generateUtilityClass.ts
index bf8fcc9c03..9b1019140e 100644
--- a/packages/material-ui-unstyled/src/generateUtilityClass/generateUtilityClass.ts
+++ b/packages/material-ui-unstyled/src/generateUtilityClass/generateUtilityClass.ts
@@ -1,6 +1,7 @@
 const globalPseudoClassesMapping: Record<string, string> = {
   active: 'Mui-active',
   checked: 'Mui-checked',
+  completed: 'Mui-completed',
   disabled: 'Mui-disabled',
   error: 'Mui-error',
   focused: 'Mui-focused',
diff --git a/packages/material-ui/src/styles/createTheme.js b/packages/material-ui/src/styles/createTheme.js
index 8572e2a46a..ae0f3a7e3b 100644
--- a/packages/material-ui/src/styles/createTheme.js
+++ b/packages/material-ui/src/styles/createTheme.js
@@ -40,6 +40,7 @@ function createTheme(options = {}, ...args) {
     const pseudoClasses = [
       'active',
       'checked',
+      'completed',
       'disabled',
       'error',
       'focused',

This would trigger the warning, like https://codesandbox.io/s/stoic-bird-vjr2w?file=/src/App.tsx

Capture d鈥檈虂cran 2021-06-24 a虁 23 34 23


The docs is already almost correct: https://next.material-ui.com/api/step-icon/#css

Capture d鈥檈虂cran 2021-06-24 a虁 23 35 20

But not quite

diff --git a/docs/scripts/buildApi.ts b/docs/scripts/buildApi.ts
index a802d95f22..906cd99365 100644
--- a/docs/scripts/buildApi.ts
+++ b/docs/scripts/buildApi.ts
@@ -32,9 +32,9 @@ import {
   renderInline as renderMarkdownInline,
 } from 'docs/src/modules/utils/parseMarkdown';
 import { pageToTitle } from 'docs/src/modules/utils/helpers';
-import createGenerateClassName from '@material-ui/styles/createGenerateClassName';
 import * as ttp from 'typescript-to-proptypes';
 import { getLineFeed, getUnstyledFilename } from './helpers';
+import { generateUtilityClass } from '@material-ui/unstyled';

 const apiDocsTranslationsDirectory = path.resolve('docs', 'translations', 'api-docs');
 function resolveApiDocsTranslationsComponentDirectory(component: ReactApi): string {
@@ -81,8 +81,6 @@ interface DescribeablePropDescriptor {

 const cssComponents = ['Box', 'Grid', 'Typography'];

-const generateClassName = createGenerateClassName();
-
 function getDeprecatedInfo(type: PropTypeDescriptor) {
   const marker = /deprecatedPropType\((\r*\n)*\s*PropTypes\./g;
   const match = type.raw.match(marker);
@@ -817,18 +815,7 @@ async function buildDocs(options: {
     reactApi.styles.name = generateMuiName(reactApi.name);
   }
   reactApi.styles.classes.forEach((key) => {
-    reactApi.styles.globalClasses[key] = generateClassName(
-      // @ts-expect-error
-      {
-        key,
-      },
-      {
-        options: {
-          name: reactApi.styles.name || generateMuiName(name),
-          theme: {},
-        },
-      },
-    );
+    reactApi.styles.globalClasses[key] = generateUtilityClass(`Mui${name}`, key);
   });

   const propErrors: Array<[propName: string, error: Error]> = [];

@liltay2

This comment has been minimized.

@github-actions github-actions bot added the duplicate This issue or pull request already exists label Jun 24, 2021
@oliviertassinari oliviertassinari removed the duplicate This issue or pull request already exists label Jun 24, 2021
michal-perlakowski added a commit to michal-perlakowski/material-ui that referenced this issue Jun 25, 2021
@siriwatknp
Copy link
Member

siriwatknp commented Jun 28, 2021

Summary of this issue

to customize completed state for StepIcon, use pseudo class stepIconClasses.completed or 'Mui-completed'.

MuiStepIcon: {
  styleOverrides: {
    root: {
      '&.Mui-completed': {
        color: 'forestgreen',
      }
    },
  },
},

@mnajdova
Copy link
Member

So, is there another action we need to do, or can we close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 馃悰 Something doesn't work component: stepper This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants