Skip to content

[Slider][material-next] Slider TS improvements#38021

Closed
DiegoAndai wants to merge 7 commits intomui:masterfrom
DiegoAndai:material-you-slider-ts
Closed

[Slider][material-next] Slider TS improvements#38021
DiegoAndai wants to merge 7 commits intomui:masterfrom
DiegoAndai:material-you-slider-ts

Conversation

@DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Jul 18, 2023

Following up on this comment: #37853 (comment)

Improve and fix slider types:

  • Move Slider type definition from Slider.types.ts file to Slider.tsx file, fixing the slotProps not being properly type checked. This should've been done when migrating Slider.js to Slider.tsx.
  • Removed slots types definitions in Slider.types.ts. This should've been done when migrating Slider.js to Slider.tsx.
  • Added more typescript testing to Slider.spec.tsx file
  • Migrate Slider.test.js tests to Slider.test.tsx

Codesandbox example of slotProps type checking: https://codesandbox.io/s/slider-slotprops-type-check-shared-6rshpl

@DiegoAndai DiegoAndai added scope: slider Changes related to the slider. typescript v6.x labels Jul 18, 2023
@DiegoAndai DiegoAndai self-assigned this Jul 18, 2023
@mui-bot
Copy link

mui-bot commented Jul 18, 2023

@DiegoAndai
Copy link
Member Author

DiegoAndai commented Jul 18, 2023

@oliviertassinari I was thinking of removing the default slots proptypes and exports, for example the default root slot proptypes and export.

Joy UI's Slider does not have the default slots proptypes, should we follow that in Material as well? Is there a reason to keep them?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jul 27, 2023
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged. and removed PR: out-of-date The pull request has merge conflicts and can't be merged. labels Aug 2, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Aug 3, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 3, 2023

Joy UI's Slider does not have the default slots proptypes, should we follow that in Material as well? Is there a reason to keep them?

  1. 👍 to eventually have the same solution between Material UI and Joy UI. It wouldn't make sense to me to approach this differently in the long run but only differently as a transition phase to learn what's best.
  2. https://mui.com/material-ui/react-slider/#customization has an example use case for exporting SliderThumb to later recompose with it.
  3. I suspect that exposing all of these nodes, so developers can compose with them would get us one step closer to the one DOM node <> one public React component optimum DX, ejecting the code being the most extreme API in this direction.
  4. In the doubt, we could keep the export limited to what we already support, to not have any BCs, which in practice, is "almost" all the slots we have in Material UI 😁, by almost, I mean, the cases where we don't, we mostly already do indirectly. Or we could remove these exports, and wait for developers to motive why we should expose them back. Happy either way.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 1, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Sep 22, 2023
@DiegoAndai DiegoAndai added v7.x and removed v6.x labels Dec 26, 2023
@DiegoAndai
Copy link
Member Author

PR got out of date as it has been on hold for a while. Closing as stale.

We'll have to revisit this later this year while working on v7.

@DiegoAndai DiegoAndai closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: out-of-date The pull request has merge conflicts and can't be merged. scope: slider Changes related to the slider. typescript v7.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments