[materia-ui][StepIcon] Add SvgIconOwnProps type to StepIcon props by sai6855 · Pull Request #44337 · mui/material-ui

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

  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

 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

DiegoAndai

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

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

 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?

…to step-icon

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

+  // 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 type: bug It doesn't behave as expected. scope: stepper Changes related to the stepper. typescript package: material-ui 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

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

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

Comment thread packages/mui-material/src/StepIcon/StepIcon.d.ts Outdated

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

oliviertassinari reviewed Nov 15, 2024

View reviewed changes

	<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'>,

Uh oh!

Conversation

sai6855 commented Nov 7, 2024

Uh oh!

mui-bot commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Bundle size report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sai6855 Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DiegoAndai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DiegoAndai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sai6855 commented Nov 14, 2024

Uh oh!

DiegoAndai commented Nov 14, 2024

Uh oh!

oliviertassinari left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mui-bot commented Nov 7, 2024 •

edited

Loading

sai6855 Nov 7, 2024 •

edited

Loading

DiegoAndai left a comment •

edited

Loading