Skip to content

Implement Slider and RangeSlider components #1083

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

Merged
merged 40 commits into from
Oct 6, 2020
Merged

Conversation

mischnic
Copy link
Contributor

@mischnic mischnic commented Sep 22, 2020

Closes #1070, Closes #1096

Haven't added docs yet so that this can be merged without having the website update before the package is released...

Known Bugs

Spectrum/CSS

  • there is no design spec for a vertical slider?
    -> design said: doesn't exist yet
  • text reflows as value label changes (both because of non-constant width and when number of digits change)

@adobe-bot

This comment has been minimized.

@devongovett
Copy link
Member

Feedback from design:

  • On text reflowing: we should attempt to reserve space for the longest possible number. By default, we can run the formatter over the minValue and maxValue props and choose the longest formatted string from that. Then we can set the width to ${maxChars}ch or maybe ${maxChars}em, whichever seems to work best. They also agreed to enabling the tnum font feature setting. This will help by making all of the numbers the same width. It won't be perfect because the period, percent sign, or unit may be different widths, but hopefully it will be close enough in most cases. As long as no characters are larger than the standard width, it should be ok if there is a little extra padding.

  • In regards to active state: the track itself shouldn't have an active state, but the thumb should have one. See the slider XD file (download from https://spectrum.adobe.com/page/slider/). Also, the cursor should be the standard arrow cursor rather than the grabber hand.

  • I think the label on the left for labelPosition: side needs a bit of padding on the right before the slider. Also according to the mocks, it looks like the bar should be the same width in this case as when the label is on top, but the overall width including the label should get wider. Right now it looks like the bar shrinks to fit the label.

  • Feel free to submit PRs to spectrum-css for the issues you found above.

@devongovett
Copy link
Member

btw, I'm guessing that the CSS for slider that's currently in the repo is a bit out of date. You could see if updating it from the spectrum-css repo fixes any of the RTL issues...

@mischnic
Copy link
Contributor Author

On text reflowing: we should attempt to reserve space for the longest possible number

Done

In regards to active state: the track itself shouldn't have an active state, but the thumb should have one. See the slider XD file (download from spectrum.adobe.com/page/slider). Also, the cursor should be the standard arrow cursor rather than the grabber hand.

Done & opened CSS pr: adobe/spectrum-css#967

I think the label on the left for labelPosition: side needs a bit of padding on the right before the slider

Done

Also according to the mocks, it looks like the bar should be the same width in this case as when the label is on top, but the overall width including the label should get wider. Right now it looks like the bar shrinks to fit the label.

The whole component takes 100% of the parent component. So with the side labels, the parent element would have to be wider than without the side labels. Making the slider container be width: calc(100% + 2 * (the label widths....)) seems like a bad idea?


One of the problems was caused by the storybook styles, not sure if this was needed for other reasons? 563b352 (#1083)

We might need to change the API to allow a trackBackgroundRTL prop (see adobe/spectrum-css#521 (comment)).

@adobe-bot

This comment has been minimized.

@adobe-bot

This comment has been minimized.

@adobe-bot

This comment has been minimized.

@adobe-bot

This comment has been minimized.

@mischnic mischnic marked this pull request as ready for review September 23, 2020 16:32
@devongovett
Copy link
Member

Looking good! One thing I noticed about the active state is that it doesn't apply when you click on the track rather than on the handle itself. You might need to make that a class and apply it with JS rather than an :active?

The whole component takes 100% of the parent component. So with the side labels, the parent element would have to be wider than without the side labels. Making the slider container be width: calc(100% + 2 * (the label widths....)) seems like a bad idea?

Maybe taking up 100% of the width of the parent by default doesn't make sense? Maybe the slider bar itself should have a default width, and then the labels could be additional width to that. We should try to match the behavior of textfield and other field components. CSS for that here: https://github.com/adobe/react-spectrum/blob/main/packages/@adobe/spectrum-css-temp/components/fieldlabel/index.css#L60-L85

We might need to change the API

Yeah I think that might be the best way. The CSS syntax is probably a bit too flexible anyway given that you could change the angle of the gradient (e.g. make it go from top-to-bottom rather than only left-to-right). Maybe we should change the prop to trackGradient specifically and accept a list of color stops. Then we can generate the CSS string based on that and flip it in RTL.

// Basic gradient
trackGradient={['red', 'green']}

// With percentages
trackGradient={['red 20%', 'green 40%']}

These could generate the following CSS:

linear-gradient(to right, red, green);
linear-gradient(to right, red 20%, green 20%);

And in RTL we could flip the to right to to left.

@adobe-bot

This comment has been minimized.

@adobe-bot

This comment has been minimized.

@devongovett
Copy link
Member

FYI: Reserving space for the value label doesn't seem to be working anymore? Looks like because of the change from min-width to width. Not sure why...

@mischnic
Copy link
Contributor Author

Good catch, I forgot to check the normal label position after fixing the side label (where min-width didn't work)....

@mischnic mischnic force-pushed the mischnic/slider branch 2 times, most recently from 2ee59a7 to a87f6c4 Compare September 25, 2020 08:43
@adobe-bot

This comment has been minimized.

@adobe-bot

This comment has been minimized.

@adobe-bot
Copy link

Build successful! 🎉

@devongovett devongovett merged commit eeb0b57 into main Oct 6, 2020
@devongovett devongovett deleted the mischnic/slider branch October 6, 2020 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants