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

[Stepper] Add componentsProps.label to StepLabel #26304

Closed
1 task done
cjduncana opened this issue May 15, 2021 · 5 comments · Fixed by #26807
Closed
1 task done

[Stepper] Add componentsProps.label to StepLabel #26304

cjduncana opened this issue May 15, 2021 · 5 comments · Fixed by #26807
Labels
component: stepper This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@cjduncana
Copy link

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

I want to add a TypographyProps argument to StepLabelProps in the same vein as StepIconProps.

Examples 🌈

How I currently code

<StepLabel>
  <span aria-current="step">Label</span>
</StepLabel>

How I wish to code

<StepLabel TypographyProps={{ 'aria-current': 'step' }}>Label</StepLabel>

Motivation 🔦

I want to add an ARIA property to the span that wraps the label, but I currently can't. My current solution is to place a span inside the StepLabel, but it just adds an unnecessary DOM element.

@cjduncana cjduncana added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 15, 2021
@cjduncana
Copy link
Author

Another possible solution to my direct problem is to have already that aria-current="step" when active is true in the StepLabel component on the Typography component. Both solutions are improvements to the component, and I'm willing to create a PR if y'all OK with these ideas.

@cjduncana
Copy link
Author

I just tried using Apple's screen reader, VoiceOver, for the first time. I just realized that it reads the current step only if I place aria-current="step" on the StepButton component. Making my use case for this feature request obsolete.

I still believe that we should add TypographyProps to the StepLabel component. Do y'all think it's still a good idea?

I suggested adding aria-current="step" to the StepLabel component in the previous comment. Given what I just learned, should we add that property to the StepButton component instead?

@oliviertassinari
Copy link
Member

This looks close to #19696 (comment)

@cjduncana
Copy link
Author

Yeah, I do see the similarities, @oliviertassinari. I'll write up a PR similar to #25883 but for the StepLabel component. What about the other question I have about aria-current="step"? Should I create a PR to add that to the StepButton component when it is active?

@mnajdova mnajdova added component: stepper This is the name of the generic UI component, not the React module! new feature New feature or request labels May 17, 2021
@oliviertassinari oliviertassinari removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 22, 2021
@oliviertassinari
Copy link
Member

A couple of thoughts:

  1. I'm not sure that the accessibility dimension of the Stepper has been seriously considered in the past. I could find different approaches. But I'm not sure we need to change it:
  1. The TypographyProps goes in opposition to the direction we took in [RFC] What's the best API to override deep/nested elements? #21453. Better follow [TextField] Allow to customize Typography in FormControlLabel #25883.
  2. Developers can't really customize the componentsProps prop in theme.components.MuiStepLabel.defaultProps, as the structure is nested. Maybe we don't need to worry about it.
  3. We could reduce the number of rendered components with:
diff --git a/packages/material-ui/src/StepLabel/StepLabel.js b/packages/material-ui/src/StepLabel/StepLabel.js
index b2749f1671..414ba72ebe 100644
--- a/packages/material-ui/src/StepLabel/StepLabel.js
+++ b/packages/material-ui/src/StepLabel/StepLabel.js
@@ -4,7 +4,6 @@ import clsx from 'clsx';
 import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
 import experimentalStyled from '../styles/experimentalStyled';
 import useThemeProps from '../styles/useThemeProps';
-import Typography from '../Typography';
 import StepIcon from '../StepIcon';
 import StepperContext from '../Stepper/StepperContext';
 import StepContext from '../Step/StepContext';
@@ -64,11 +63,13 @@ const StepLabelRoot = experimentalStyled('span', {
   }),
 }));

-const StepLabelLabel = experimentalStyled(Typography, {
+const StepLabelLabel = experimentalStyled('span', {
   name: 'MuiStepLabel',
   slot: 'Label',
   overridesResolver: (props, styles) => styles.label,
 })(({ theme }) => ({
+  ...theme.typography.body2,
+  display: 'block',
   /* Styles applied to the Typography component that wraps `children`. */
   transition: theme.transitions.create('color', {
     duration: theme.transitions.duration.shortest,
@@ -170,9 +171,6 @@ const StepLabel = React.forwardRef(function StepLabel(inProps, ref) {
       <StepLabelLabelContainer className={classes.labelContainer} styleProps={styleProps}>
         {children ? (
           <StepLabelLabel
-            variant="body2"
-            component="span"
-            display="block"
             className={classes.label}
             styleProps={styleProps}
           >

@oliviertassinari oliviertassinari changed the title [Stepper] Add TypographyProps to StepLabel [Stepper] Add componentsProps.label to StepLabel May 22, 2021
@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label May 22, 2021
michal-perlakowski added a commit to michal-perlakowski/material-ui that referenced this issue Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: stepper This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants