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

[InputLabel] Add missing component type #35852

Merged
merged 19 commits into from
Jan 30, 2023
Merged

[InputLabel] Add missing component type #35852

merged 19 commits into from
Jan 30, 2023

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Jan 17, 2023

@sai6855 sai6855 marked this pull request as draft January 17, 2023 13:14
@mui-bot
Copy link

mui-bot commented Jan 17, 2023

Messages
📖 Netlify deploy preview: https://deploy-preview-35852--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against 357c222

@zannager zannager added the component: FormControl The React component label Jan 17, 2023
@sai6855
Copy link
Contributor Author

sai6855 commented Jan 18, 2023

@ZeeshanTamboli can you please review this PR

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work typescript labels Jan 19, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a TypeScript test case by creating a file named InputLabel.spec.tsx.

@ZeeshanTamboli ZeeshanTamboli added the PR: needs test The pull request can't be merged label Jan 19, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder that we can simply add component type in the InputLabelProps interface:

--- a/packages/mui-material/src/InputLabel/InputLabel.d.ts
+++ b/packages/mui-material/src/InputLabel/InputLabel.d.ts
@@ -18,6 +18,11 @@ export interface InputLabelProps extends StandardProps<FormLabelProps> {
    */
   classes?: Partial<InputLabelClasses>;
   color?: FormLabelProps['color'];
+  /**
+   * The component used for the root node.
+   * Either a string to use a HTML element or a component.
+   */
+  component?: React.ElementType;
   /**
    * If `true`, the transition animation is disabled.
    * @default false

and that would be enough instead of having all these type wrappers.

@ZeeshanTamboli ZeeshanTamboli removed the PR: needs test The pull request can't be merged label Jan 19, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [Inputlabel] Fix component type issue [InputLabel] Fix component type issue Jan 19, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [InputLabel] Fix component type issue [InputLabel] Fix missing component type Jan 19, 2023
@sai6855
Copy link
Contributor Author

sai6855 commented Jan 19, 2023

I wonder that we can simply add component type in the InputLabelProps interface:

--- a/packages/mui-material/src/InputLabel/InputLabel.d.ts
+++ b/packages/mui-material/src/InputLabel/InputLabel.d.ts
@@ -18,6 +18,11 @@ export interface InputLabelProps extends StandardProps<FormLabelProps> {
    */
   classes?: Partial<InputLabelClasses>;
   color?: FormLabelProps['color'];
+  /**
+   * The component used for the root node.
+   * Either a string to use a HTML element or a component.
+   */
+  component?: React.ElementType;
   /**
    * If `true`, the transition animation is disabled.
    * @default false

and that would be enough instead of having all these type wrappers.

Then i think following test would fail and it is expected to pass.

https://github.com/mui/material-ui/pull/35852/files#diff-f5beb3d0a6cd307137bc2c29e830648a962b649fb44ea6b0221f9d0360f8cadaR18

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jan 19, 2023

I wonder that we can simply add component type in the InputLabelProps interface:

--- a/packages/mui-material/src/InputLabel/InputLabel.d.ts
+++ b/packages/mui-material/src/InputLabel/InputLabel.d.ts
@@ -18,6 +18,11 @@ export interface InputLabelProps extends StandardProps<FormLabelProps> {
    */
   classes?: Partial<InputLabelClasses>;
   color?: FormLabelProps['color'];
+  /**
+   * The component used for the root node.
+   * Either a string to use a HTML element or a component.
+   */
+  component?: React.ElementType;
   /**
    * If `true`, the transition animation is disabled.
    * @default false

and that would be enough instead of having all these type wrappers.

Then i think following test would fail and it is expected to pass.

https://github.com/mui/material-ui/pull/35852/files#diff-f5beb3d0a6cd307137bc2c29e830648a962b649fb44ea6b0221f9d0360f8cadaR18

Oh yes. Thanks for the test. 👍 We need to get props of the component if component={Component} is used.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sai6855 This works well 👍. But because InputLabel inherits FormLabel component, I realised that we need to extend the types with the Extend keyword as per convention.

For this, as an example, you can refer to the Button component types in Button.d.ts file, where the Button component inherits ButtonBase component. You can take a look at other components as well, where one component inherits another. I am sorry; I should have said it earlier.

@sai6855
Copy link
Contributor Author

sai6855 commented Jan 23, 2023

@sai6855 This works well 👍. But because InputLabel inherits FormLabel component, I realised that we need to extend the types with the Extend keyword as per convention.

For this, as an example, you can refer to the Button component types in Button.d.ts file, where the Button component inherits ButtonBase component. You can take a look at other components as well, where one component inherits another. I am sorry; I should have said it earlier.

Ah okay, i didn't noticed there is already a convention for this use-case. As suggested migrated code as per existing convention.

https://github.com/mui/material-ui/pull/35852/files#diff-40c72c413c58d6ccab3754f4c54a76b05d428fffc892c85f66bae5b290934912R75-R79

@ZeeshanTamboli ZeeshanTamboli changed the title [InputLabel] Fix missing component type [InputLabel] Add missing component type Jan 24, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks @sai6855 for your contribution.

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: FormControl The React component typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants