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 track="inverted"> does not highlight the currently-selected mark #18929

Closed
1 task done
seansfkelley opened this issue Dec 20, 2019 · 5 comments · Fixed by #18993
Closed
1 task done

<Slider track="inverted"> does not highlight the currently-selected mark #18929

seansfkelley opened this issue Dec 20, 2019 · 5 comments · Fixed by #18993
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@seansfkelley
Copy link

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

When using discrete marks with the forward slider, marks are highlighted if the slider is at or above their value.

When using discrete marks with the inverted slider, marks are highlighted if the slider is strictly above their value.

Examples 🌈

Using sliders from the slider docs.

A forward slider at exactly 37 degrees:

image

An inverted slider at exactly 37 degrees:

image

An inverted slider at maximum of 100 degrees has no highlights:

image

Motivation 🔦

In my case, I have a Low/Medium/High slider, and it's being used to set a minimum threshold for importance. The threshold is inclusive, but the visuals don't suggest that. It makes sense to be inclusive in this case, because it would not make sense for an exclusive slider to be on High as nothing would match that filter.

@mbrookes mbrookes added component: slider This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work labels Dec 20, 2019
@mbrookes
Copy link
Member

@seansfkelley What do you think of this diff?

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 99663febd..0ec32c258 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -741,16 +741,19 @@ const Slider = React.forwardRef(function Slider(props, ref) {
       {marks.map(mark => {
         const percent = valueToPercent(mark.value, min, max);
         const style = axisProps[axis].offset(percent);
-
         let markActive;
         if (track === false) {
           markActive = values.indexOf(mark.value) !== -1;
         } else {
-          const isMarkActive = range
-            ? mark.value >= values[0] && mark.value <= values[values.length - 1]
-            : mark.value <= values[0];
           markActive =
-            (isMarkActive && track === 'normal') || (!isMarkActive && track === 'inverted');
+            (track === 'normal' &&
+              (range
+                ? mark.value >= values[0] && mark.value <= values[values.length - 1]
+                : mark.value <= values[0])) ||
+            (track === 'inverted' &&
+              (range
+                ? mark.value <= values[0] || mark.value >= values[values.length - 1]
+                : mark.value >= values[0]));
         }
 
         return (

image

@oliviertassinari
Copy link
Member

👍 for adding a regression test too

@seansfkelley
Copy link
Author

I haven't thought through any potential edge cases in that diff, but using non-strict comparison operators for both kinds seems to be the key change, so looks good to me! Thanks for the quick turnaround.

@mbrookes
Copy link
Member

No worries, would you like to submit a PR?

@mbrookes mbrookes added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 21, 2019
@ghost
Copy link

ghost commented Dec 23, 2019

I can take care of this, if I may.
I'll be working to submit this PR.

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! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants