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] Prevent rendering for marks that are out of min&max bounds #32436

Merged

Conversation

abriginets
Copy link
Contributor

This fixes an issue when Slider component renders marks that are out of min and max bounds, e.g. when user wants to render a mark for 120 value when min is 0 and max is 100. See CodeSandbox for reference.

P.S. - sorry for posting this PR twice. I accidentally checked out into outdated version of mui.

@mui-bot
Copy link

mui-bot commented Apr 23, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 3f4ba1e

@danilo-leal danilo-leal added the component: slider This is the name of the generic UI component, not the React module! label Apr 26, 2022
@mnajdova
Copy link
Member

mnajdova commented Apr 28, 2022

The changes makes sense to me 👍 It's easier to review by hiding whitespace - https://github.com/mui/material-ui/pull/32436/files?diff=split&w=1

Considering the base Slider is used in the Mateiral UI slider (which is unfortunate at this moment), I would say this is a breaking change, but I am willing to push this, as I would say is a bug fix, rather than change in behavior. cc @michaldudak what are your thoughts on this?


I remember that there was an issue for this, could you please share it in the PR description?

@michaldudak
Copy link
Member

I agree - rendering marks outside of the slider range doesn't look like a feature someone would depend on.

Side note:
This, however, gave me an idea of another feature we could implement one day - decouple rendered min and max from values available to select - so a slider could always be rendered from, say, 0 to 100, but values available to select could be restricted to 10-90. It could be handy where min and max are set dynamically in relation to another value and we want to show that in certain conditions selection of values outside of the currently available range is possible.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks great, I believe we can merge this. Thanks @abriginets

@mnajdova mnajdova merged commit 58d5a9d into mui:master May 19, 2022
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants