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

[Select] Add extending OutlinedInputProps by SelectProps #31209

Merged
merged 2 commits into from Mar 1, 2022
Merged

[Select] Add extending OutlinedInputProps by SelectProps #31209

merged 2 commits into from Mar 1, 2022

Conversation

jrozbicki
Copy link
Contributor

@jrozbicki jrozbicki commented Feb 25, 2022

Closes #31208
According to docs Select should have OutlineInput props available. Functionality works, but typescript coverage was lacking.

@jrozbicki

This comment was marked as resolved.

@mui-bot
Copy link

mui-bot commented Feb 25, 2022

No bundle size changes

Generated by 🚫 dangerJS against 5040455

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 I think adding notched?: boolean to the SelectProps interface should be enough (no need to extend the OutlinedInputProps again because it extends StandardProps).

Also, please add type test to Select.spec.tsx to make sure that it is working.

@jrozbicki
Copy link
Contributor Author

jrozbicki commented Feb 28, 2022

@siriwatknp

👍 I think adding notched?: boolean to the SelectProps interface should be enough (no need to extend the OutlinedInputProps again because it extends StandardProps).

I see your point, however, according to docs Props of the OutlinedInput component are also available.

If OutlinedInput props were ever to change, then Select props would fall out of sync. I see that now only notched is what's different, but both InputProps and OutlineInputProps extends StandardProps<InputBaseProps> I don't see a harm in that.

Let me know what you think and I'll add tests in meantime.

@danilo-leal danilo-leal changed the title add extending OutlinedInputProps by SelectProps (#31208) [Select] Add extending OutlinedInputProps by SelectProps Feb 28, 2022
@danilo-leal danilo-leal added component: select This is the name of the generic UI component, not the React module! typescript labels Feb 28, 2022
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 truly appreciate your contribution!

@siriwatknp
Copy link
Member

If OutlinedInput props were ever to change, then Select props would fall out of sync. I see that now only notched is what's different, but both InputProps and OutlineInputProps extends StandardProps<InputBaseProps> I don't see a harm in that.

Yep, you are right about it. Thanks for clarifying.

@siriwatknp siriwatknp merged commit f64fe3b into mui:master Mar 1, 2022
@jrozbicki jrozbicki deleted the patch-1 branch March 1, 2022 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Typescript] Select doesn't accept notched prop (missing extending OutlinedInputProps)
4 participants