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] Fix incorrect marks displayed due to duplicate keys in range #33526

Merged
merged 3 commits into from Sep 30, 2022

Conversation

kskd1804
Copy link
Contributor

@kskd1804 kskd1804 commented Jul 15, 2022

Fixes #31960

Description of the issue

Issue is caused when slider has more than one mark with the same value causing a duplicate key error. For example, when the lower limit and the upper limit thumbs in the range slider (marks are displayed for the thumb positions) are placed at the same position 100, both thumbs have the same key 100, causing issues with rendering sliders marks correctly.

What fix was adopted?

The fix was to replace the key value to mark index instead of mark value.

@mui-bot
Copy link

mui-bot commented Jul 15, 2022

Details of bundle changes

Generated by 🚫 dangerJS against fe08cdb

@michaldudak
Copy link
Member

Thanks! Could you please check if this issue appears also in unit test and, if yes, create a test that verifies the fix?

@oliviertassinari oliviertassinari added component: slider This is the name of the generic UI component, not the React module! PR: needs test The pull request can't be merged labels Sep 9, 2022
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.

It's a great first pull request in MUI 👌 Thanks a lot for working on it! I've added a test to ensure we won't create a regression in the future.

@mnajdova mnajdova changed the title Fixed incorrect marks displayed due to duplicate keys in range slider [Slider] Fixed incorrect marks displayed due to duplicate keys in range Sep 29, 2022
@mnajdova mnajdova merged commit 7b54770 into mui:master Sep 30, 2022
@oliviertassinari oliviertassinari changed the title [Slider] Fixed incorrect marks displayed due to duplicate keys in range [Slider] Fix incorrect marks displayed due to duplicate keys in range Sep 30, 2022
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed PR: needs test The pull request can't be merged labels Sep 30, 2022
@oliviertassinari
Copy link
Member

Nice bug fix 👌, I didn't anticipate this when I wrote the logic in #15703. I cleaned up a bit the label and PR title

alexfauquette pushed a commit to alexfauquette/material-ui that referenced this pull request Oct 14, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
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: 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.

[Slider] Remove not needed mark in range Sliders
5 participants