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

[Slider][material][base] Allow generic value type to narrow onChange value type #38127

Closed
wants to merge 6 commits into from
Closed

Conversation

gitstart
Copy link
Contributor

@gitstart gitstart commented Jul 23, 2023

Description

Narrow Slider onChange value type to be either number or number[] depending on the type of the value argument.

Updated onChange prop in:

packages/mui-base/src/useSlider/useSlider.types.ts
packages/mui-material-next/src/Slider/Slider.types.ts
packages/mui-material/src/Slider/Slider.d.ts

Closes #37854


This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.

@mui-bot
Copy link

mui-bot commented Jul 23, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against ee1935e

@gitstart gitstart marked this pull request as ready for review July 26, 2023 09:23
@gitstart
Copy link
Contributor Author

Hi @DiegoAndai this PR is ready for review.

Note that I used a union of different call signatures to define the slider onChange type instead of a generic. WDYT?

@zannager zannager added the component: slider This is the name of the generic UI component, not the React module! label Jul 26, 2023
@zannager zannager requested a review from michaldudak July 26, 2023 13:52
@DiegoAndai DiegoAndai self-requested a review July 27, 2023 15:17
@DiegoAndai DiegoAndai changed the title MUI-37854 - [Slider] Allow generic value type to narrow onChange value type [Slider][material][base] Allow generic value type to narrow onChange value type Jul 27, 2023
@DiegoAndai
Copy link
Member

Hi @gitstart! thanks for working on this

Sadly, I don't think this is solving the issue we want to solve 😕

Look at this codesandbox: https://codesandbox.io/s/slider-onchange-type-pr-37854-llcchv?file=/src/App.tsx

In the example, the slider is "single value" (as the value is a number). The handleChange function is typed as receiving a number[], so what we want with this fix is that handleChange throws an error when passed to onChange, as onChange should receive the same type as the value, which is number. Does that make sense?

We either have to extract the type from the value prop (I don't know if this one is possible) or transform the Slider into a generic component.

@michaldudak what's your take on this?

@gitstart
Copy link
Contributor Author

@DiegoAndai thanks for the feedback. We actually tried exploring the first suggestion (extracting the type from the value prop), but it didn't work out. I think it's worth trying out your second suggestion.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 30, 2023
@gitstart gitstart closed this Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slider] Allow generic value type to narrow onChange value type
4 participants