[materia-ui][StepIcon] Add SvgIconOwnProps type to StepIcon props #44337

sai6855 · 2024-11-07T05:15:03Z

closes: #44328

I have followed (at least) the PR section of the contributing guide.

mui-bot · 2024-11-07T05:17:59Z

Netlify deploy preview

https://deploy-preview-44337--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against cc53e00

sai6855 · 2024-11-07T05:48:50Z

packages/mui-material/src/StepIcon/StepIcon.test.js

 import describeConformance from '../../test/describeConformance';

 describe('<StepIcon />', () => {
  const { render } = createRenderer();

  describeConformance(<StepIcon icon={1} />, () => ({
    classes,
-    inheritComponent: 'svg',
+    inheritComponent: SvgIcon,


this change is to display Props of the SvgIcon component are also available. in props table.

https://deploy-preview-44337--material-ui.netlify.app/material-ui/api/step-icon/#props

sai6855 · 2024-11-07T05:52:13Z

packages/mui-material/src/StepIcon/StepIcon.d.ts

 import { Theme } from '../styles';
 import { StepIconClasses } from './stepIconClasses';

 export interface StepIconProps
-  extends StandardProps<React.HTMLAttributes<HTMLDivElement>, 'children'> {
+  extends StandardProps<React.HTMLAttributes<HTMLDivElement>, 'children'>,
+    Omit<SvgIconOwnProps, 'color' | 'children'> {


All the props which are passed to StepIcon except StepIconProps are passed to StepIconRoot which is inherited from SvgIcon. so extending SvgIconOwnProps here seemed obvious.

Omitted color due to conflicting types

material-ui/packages/mui-material/src/StepIcon/StepIcon.js

Lines 79 to 85 in 412dcbf

<StepIconRoot

as={Warning}

className={className}

ref={ref}

ownerState={ownerState}

{...other}

/>

I think we should use SvgIconProps instead of SvgIconOwnProps, or is there a reason to use the later?

What is the color conflict?

I think we should use SvgIconProps instead of SvgIconOwnProps, or is there a reason to use the later?

Reason i went for SvgIconOwnProps instead of SvgIconProps is due to conflicting event handler types. For example type of onClick event handler is different in React.HTMLAttributes<HTMLDivElement> and SvgIconProps, since both are different types, typescript is unable to extend both types. Attached playground which explains the issue better

https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgMqQA4EkEHsQAKUuGAzshAB6QgAm5AShImAHQASAKgLIAyAgmDBRgAIwCukUgB4ufACLAAbgFEANhAC2EcAD4ANMiYsOPAUJESp01ADUA4nfvqtOsLuQBvAFDeAvkA

What is the color conflict?

type of color in React.HTMLAttributes<HTMLDivElement> is string | undefined where as type of color in ` color?: OverridableStringUnion<
| 'inherit'
| 'action'
| 'disabled'
| 'primary'
| 'secondary'
| 'error'
| 'info'
| 'success'
| 'warning',
SvgIconPropsColorOverrides

;due to this conflict i've omittedcolor` type

Now that i think about this, since color prop passed to StepIcon gets passed to SvgIcon i think we should omit color from React.HTMLAttributes<HTMLDivElement> not from SvgIconOwnProps. what do you think? If we make color type more strict, would it be breaking change for users who didn't typed properply?

Reason i went for SvgIconOwnProps instead of SvgIconProps is due to conflicting event handler types.

I see. And is React.HTMLAttributes<HTMLDivElement> used correctly here? Shouldn't it be React.HTMLAttributes<SVGSVGElement>? The root is an SVG, right?

type of color in React.HTMLAttributes is string | undefined

Same question that the one above. Did string work? Or do only the SvgIconOwnProps.color values work?

Shouldn't it be React.HTMLAttributes?

Initially i thought of doing this, but thought it would be breaking change for users if they depend on React.HTMLAttributes<HTMLDivElement>

Anyway, i updated types here 463efe7 11a1807

packages/mui-material/src/StepIcon/StepIcon.spex.tsx

DiegoAndai

Thanks for working on this @sai6855! a couple of comments:

DiegoAndai · 2024-11-08T15:48:32Z

packages/mui-material/src/StepIcon/StepIcon.d.ts

 import { Theme } from '../styles';
 import { StepIconClasses } from './stepIconClasses';

 export interface StepIconProps
-  extends StandardProps<React.HTMLAttributes<HTMLDivElement>, 'children'> {
+  extends StandardProps<React.HTMLAttributes<HTMLDivElement>, 'children'>,
+    Omit<SvgIconOwnProps, 'color' | 'children'> {


I think we should use SvgIconProps instead of SvgIconOwnProps, or is there a reason to use the later?

What is the color conflict?

packages/mui-material/src/StepIcon/StepIcon.spex.tsx

…to step-icon

packages/mui-material/src/StepIcon/StepIcon.d.ts

DiegoAndai

Hey @sai6855! Thanks for implementing the feedback.

Initially i thought of doing this, but thought it would be breaking change for users if they depend on React.HTMLAttributes

I think you're correct. I didn't consider this. Considering it, your initial approach of using SvgIconOwnProps looks like the correct solution. May I ask you to go back to that solution using SvgIconOwnProps? Let's keep the changes to a minimum to avoid breaking changes.

I'm sorry for making you redo this 😓, my bad.

sai6855 · 2024-11-14T16:56:57Z

Hey @sai6855! Thanks for implementing the feedback.

Initially i thought of doing this, but thought it would be breaking change for users if they depend on React.HTMLAttributes

I think you're correct. I didn't consider this. Considering it, your initial approach of using SvgIconOwnProps looks like the correct solution. May I ask you to go back to that solution using SvgIconOwnProps? Let's keep the changes to a minimum to avoid breaking changes.

I'm sorry for making you redo this 😓, my bad.

No issues :) , i've redid changes here 71c19e9 and added comment in this commit 923297e

DiegoAndai · 2024-11-14T17:39:00Z

Thanks @sai6855!

oliviertassinari

Nice docs improvement 👍

oliviertassinari · 2024-11-15T18:27:47Z

packages/mui-material/src/StepIcon/StepIcon.d.ts

+  // TODO v7: extend React.HTMLAttributes<SVGSVGElement> as svg is root component of StepIcon not div
+  extends StandardProps<React.HTMLAttributes<HTMLDivElement>, 'color' | 'children'>,


Why wait? Can't we fix this now? It's a bug fix, right?

I agree with it being bug, but I'm expecting there will be considerable amount of users who are rely on wrong types.

So I wasn't sure if we could fix this between major releases.

sai6855 added 2 commits November 7, 2024 10:42

add svgiconprops to stepicon interface

e513ddd

add tests

85b626e

sai6855 marked this pull request as draft November 7, 2024 05:15

sai6855 added bug 🐛 Something doesn't work component: stepper This is the name of the generic UI component, not the React module! typescript package: material-ui Specific to @mui/material labels Nov 7, 2024

sai6855 added 3 commits November 7, 2024 10:57

fix-type-issues

95da2de

fix

832243c

docs

63bf2b7

sai6855 commented Nov 7, 2024

View reviewed changes

sai6855 marked this pull request as ready for review November 7, 2024 05:52

sai6855 commented Nov 7, 2024

View reviewed changes

packages/mui-material/src/StepIcon/StepIcon.spex.tsx Outdated Show resolved Hide resolved

sai6855 requested a review from DiegoAndai November 7, 2024 05:53

DiegoAndai reviewed Nov 8, 2024

View reviewed changes

sai6855 and others added 5 commits November 11, 2024 10:46

fix file name

7fffa51

Merge branch 'master' into step-icon

085d251

fix type

463efe7

fix type

11a1807

Merge branch 'step-icon' of https://github.com/sai6855/material-ui in…

4b3aa99

…to step-icon

sai6855 commented Nov 12, 2024

View reviewed changes

packages/mui-material/src/StepIcon/StepIcon.d.ts Outdated Show resolved Hide resolved

DiegoAndai reviewed Nov 14, 2024

View reviewed changes

sai6855 and others added 3 commits November 14, 2024 21:37

update types to use SvgIconOwnProps

71c19e9

add comment

923297e

Merge branch 'master' into step-icon

cc53e00

DiegoAndai approved these changes Nov 14, 2024

View reviewed changes

DiegoAndai merged commit 3251c3a into mui:master Nov 14, 2024
22 checks passed

oliviertassinari reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[materia-ui][StepIcon] Add SvgIconOwnProps type to StepIcon props #44337

[materia-ui][StepIcon] Add SvgIconOwnProps type to StepIcon props #44337

sai6855 commented Nov 7, 2024

mui-bot commented Nov 7, 2024 •

edited

Loading

sai6855 Nov 7, 2024

sai6855 Nov 7, 2024 •

edited

Loading

DiegoAndai Nov 8, 2024

sai6855 Nov 11, 2024

DiegoAndai Nov 11, 2024

sai6855 Nov 12, 2024

DiegoAndai left a comment

DiegoAndai Nov 8, 2024

DiegoAndai left a comment •

edited

Loading

sai6855 commented Nov 14, 2024

DiegoAndai commented Nov 14, 2024

oliviertassinari left a comment

oliviertassinari Nov 15, 2024

sai6855 Nov 16, 2024

	<StepIconRoot
	as={Warning}
	className={className}
	ref={ref}
	ownerState={ownerState}
	{...other}
	/>

		// TODO v7: extend React.HTMLAttributes<SVGSVGElement> as svg is root component of StepIcon not div
		extends StandardProps<React.HTMLAttributes<HTMLDivElement>, 'color' \| 'children'>,

[materia-ui][StepIcon] Add SvgIconOwnProps type to StepIcon props #44337

[materia-ui][StepIcon] Add SvgIconOwnProps type to StepIcon props #44337

Conversation

sai6855 commented Nov 7, 2024

mui-bot commented Nov 7, 2024 • edited Loading

Netlify deploy preview

Bundle size report

Choose a reason for hiding this comment

sai6855 Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoAndai left a comment • edited Loading

Choose a reason for hiding this comment

sai6855 commented Nov 14, 2024

DiegoAndai commented Nov 14, 2024

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mui-bot commented Nov 7, 2024 •

edited

Loading

sai6855 Nov 7, 2024 •

edited

Loading

DiegoAndai left a comment •

edited

Loading