MultiRangeSlider component #2407

jscheiny · 2018-04-19T17:06:23Z

Checklist

Enable CircleCI for your fork
Include tests
Update documentation

Changes proposed in this pull request:

This adds a very general MultiRangeSlider component that ultimately should be able to subsume the Slider and RangeSlider components.

Notes about the component API:

Instead of taking a value/values the component takes a list of handles specified by props. Each handle has:
value - where it gets placed on the slider
type - specifies how the slider handle should be displayed
- "full" - handle appears like it does in a Slider (default)
- "lower" - handle appears like the first slider in a RangeSlider
- "upper" - handle appears like the second slider in a RangeSlider
trackIntent{Above,Below} specifies the color of the track fill above and below (numerically) of the given handle.

Handles are always displayed in ascending numeric order and the values that are returned by onChange and onRelease are always sorted.

In addition, MultiRangeSlider takes a defaultTrackIntent prop which is used in the resolution of how to render the color of the track between two given handles. The way that this is resolved is as follows (and is one of my concerns about the API).

If the left handle has an above intent, use that
If the right handle has a below intent, use that
If the component has a default intent, use that
Otherwise don't color that section

Important: Note that there is a difference between the NONE intent and the absence of an intent entirely (undefined). The former will render a gray track between the two handles and the latter will fall-through to the next color in the hierarchy above.

I previously allowed the specification of colors instead of intents but that didn't seem very blueprint-y.

Reviewers should focus on:

I have explicitly not written tests/docs for this yet, as I would first like to iterate on the API as I'm not convinced what I've written is particularly nice (worked for our use case but don't know if it generalizes well). Once we've iterated and nailed down an API I will gladly write tests and docs for this component.

I've written some questions in the code, but one additional question I have is should this component live in labs first to incubate before moving into core?

Not addressed in this PR (requires follow-up):

To keep this PR simple(r) and easy(er) to review, I have not made many changes to the existing slider components but in 1-2 follow up PRs I would like to:

Refactor Slider and RangeSlider to be simple wrappers around MultiRangeSlider
Remove CoreSlider and refactor into MultiRangeSlider

The second one is questionable, especially if there are plans to add a slider that does not fit into the multi range slider model.

Screenshots

Two tailed slider:

Disabled:

One tailed:

Weird:

Vertical:

In action:

palantirtech · 2018-04-19T17:06:24Z

Thanks for your interest in palantir/blueprint, @jscheiny! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

jscheiny · 2018-04-19T18:02:36Z

packages/core/src/components/slider/_slider.scss

+    border-bottom-right-radius: 0;
+  }
+
+  &.pt-intent-primary {


I'm not a sass wizard, and I imagine that there's a much nicer way of doing these intent blocks. Any thoughts?

you can loop through all the intents. check out how buttons, callouts, and other intent-supported components do it

jscheiny · 2018-04-19T18:08:32Z

packages/core/src/components/slider/handle.tsx

-        handle: (el: HTMLSpanElement) => (this.handleElement = el),
+        handle: (el: HTMLSpanElement) => {
+            this.handleElement = el;
+            this.forceUpdate();


This is here because when the number of handles changes, these handles are re-instantiated. When that happens the first render of the component does not have the ref captured and thus incorrectly positions the handle. This fixes by immediately re-rendering once the ref is captured, not sure if there's a nicer way to handle (haha) this. Otherwise, will add a comment to this effect.

llorca · 2018-04-19T19:11:54Z

one hell of a first time contrib, @jscheiny 🤯 😍

jscheiny · 2018-04-19T20:10:23Z

I like to make an entrance 😄

jscheiny · 2018-04-19T21:05:32Z

Use children instead of array of props

Preview: documentation | landing | table

jscheiny · 2018-04-19T21:06:14Z

Some minor styling issues I found:

When a lower handle with its above track having no intent is moved to the end of the slider, the border radius can be seen through slightly:

Same problem at the other end (upper handle with no below track color):

@llorca This is pretty minor, worth fixing?

llorca · 2018-04-19T21:13:46Z

packages/core/src/common/classes.ts

 export const SLIDER_PROGRESS = "pt-slider-progress";
 export const RANGE_SLIDER = "pt-range-slider";
+export const MULTI_RANGE_SLIDER = "pt-multi-range-slider";
+export const LOWER = "pt-lower";


what about start and end? we use this vocabulary in other places already (more generic), and it seems to make sense as well here. In addition, lower and upper imply a vertical axis, whereas sliders can be either horizontal or vertical

Done. Also changed "above" and "below" to "after" and "before"

llorca · 2018-04-19T21:16:00Z

@jscheiny yes please fix. an easy way would be to shave 1-2px off the max left or right a handle can be, so that it's always on top of the track border radii. I would not suggest modifying the border radius depending on the position of the handles

jscheiny · 2018-04-19T21:17:59Z

Fix lint?

Preview: documentation | landing | table

llorca · 2018-04-19T22:27:29Z

@jscheiny even though the API isn't locked down, this is clearly an improvement and we'll work with you to get it in. Can you set up a quick docs section for Multi-range slider, with just examples so we can test it out? We can write the full docs later.

…range-slider

jscheiny · 2018-04-20T14:26:34Z

Merge remote-tracking branch 'upstream/develop' into review/js/multi-range-slider

Preview: documentation | landing | table

jscheiny · 2018-04-20T15:08:00Z

Simplify intent coloring

Preview: documentation | landing | table

jscheiny · 2018-04-20T15:09:35Z

@llorca I'd be happy to add an example. However, I can't seem to get markdown changes to compile locally so I can't test that what I'm doing works. I'm running with yarn dev:docs and whenever I make changes to .md files there's no change in the UI. Any thoughts?

llorca · 2018-04-20T17:39:48Z

@jscheiny hmm yeah I have this issue sometimes too. what about yarn dev:core? if it's still doesn't work, it'll be a @giladgray question

jscheiny · 2018-04-20T18:54:52Z

I was able to do it by running yarn compile in the docs-data package after every markdown change. It would be good to know if there's a nicer way of doing this and, if so, document it.

Anyways @llorca there should be a multi-range slider example now.

jscheiny · 2018-04-20T18:56:39Z

Example component

Preview: documentation | landing | table

giladgray · 2018-04-20T21:22:23Z

@jscheiny run yarn docs-data in the root to re-compile only the docs json file. there's no watcher for it as it's like way too many files and soooo slow (30+ seconds) so i made a root task to simplify the process.

jscheiny · 2018-04-24T21:35:21Z

Remove import

Preview: documentation | landing | table

giladgray · 2018-04-19T18:14:38Z

packages/core/src/components/slider/multiRangeSlider.tsx

+
+export interface ISliderHandleProps {
+    value: number;
+    trackIntentAbove?: Intent;


className? and trackClassName? would allow custom styling (like non-intent track colors)

giladgray · 2018-04-20T21:25:49Z

packages/core/src/components/slider/handle.tsx

+        handle: (el: HTMLSpanElement) => {
+            this.handleElement = el;
+            // TODO If this ends up being the approved way of working, only call this function on the first ref and add
+            // a comment explaning why.


if it's just on the first ref, then it should be in componentDidMount

Can do that, did you see my other comment on why this is necessary / have any thoughts on how to remedy?

giladgray · 2018-04-20T21:33:19Z

packages/core/src/components/slider/multiRangeSlider.tsx

+    let minValue = values[0];
+    let minArg = argFn(minValue);
+
+    for (let index = 1; index < values.length; index++) {


Math.min(...spread an array)?

I needed something equivalent of _.minBy, i.e. find the min of an array as determined by some function.

giladgray · 2018-04-20T21:34:08Z

packages/core/src/components/slider/multiRangeSlider.tsx

+    ).filter(child => child !== null);
+}
+
+function compare(left: number, right: number) {


used once, i suggest inlining the subtraction

giladgray · 2018-04-24T22:51:42Z

packages/core/src/components/slider/multiRangeSlider.tsx

+import { ISliderHandleProps, SliderHandle } from "./sliderHandle";
+
+export interface IMultiRangeSliderProps extends ICoreSliderProps {
+    children?: React.ReactNode;


this is not necessary as it's the same as React's definition. what would be awesome, since it's validated below, is to declare something along the lines of children?: React.ReactElement<ISliderHandleProps>[]

giladgray · 2018-04-24T22:54:46Z

packages/core/src/components/slider/sliderHandle.tsx

+
+export type SliderHandleType = "full" | "start" | "end";
+
+export interface ISliderHandleProps {


className? and trackClassName? would allow custom styling (like non-intent track colors)

Not sure what className styling on the handle itself would do. Happy to do the class name suggestion, what do you think trackAboveClassName and trackBelowClassName? I found it pretty important in my usage of this component to be able to specify for certain handles the color on the track on one side or the other of it.

Should we still have intent props or just expect people to pass in Classes.INTENT_PRIMARY etc.?

giladgray · 2018-04-24T22:55:13Z

packages/core/src/components/slider/sliderHandle.tsx

+    type?: SliderHandleType;
+}
+
+export class SliderHandle extends AbstractPureComponent<ISliderHandleProps> {


can just use React.PureComponent here as you're not using any of the APC features

jscheiny · 2018-04-25T14:41:18Z

Address comments

Preview: documentation | landing | table

jscheiny · 2018-04-30T21:59:59Z

Refactor slider tests

Preview: documentation | landing | table

jscheiny · 2018-05-01T16:16:41Z

Merge branch 'develop' into js/multi-range-slider

Preview: documentation | landing | table

… into review/js/multi-range-slider

jscheiny · 2018-05-01T20:31:33Z

Merge branch 'js/multi-range-slider' of github.com:jscheiny/blueprint into review/js/multi-range-slider

Preview: documentation | landing | table

jscheiny · 2018-05-02T14:40:35Z

Add default track intent

Preview: documentation | landing | table

jscheiny · 2018-05-02T21:52:52Z

Fix vertical track border radii

Preview: documentation | landing | table

jscheiny · 2018-05-03T15:45:46Z

Fix lint

Preview: documentation | landing | table

giladgray · 2018-05-22T01:28:00Z

@jscheiny mind re-opening this as a branch in this repo so I can contribute as well?

jscheiny · 2018-05-22T14:55:28Z

Closing this in favor of #2536

Jonah Scheinerman added 2 commits April 19, 2018 10:48

Multi range slider

f1666d8

Lint and oops

d892981

jscheiny commented Apr 19, 2018

View reviewed changes

Use children instead of array of props

e4680a2

Fix lint?

6aeab97

llorca reviewed Apr 19, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into review/js/multi-…

f2c3530

…range-slider

Simplify intent coloring

07d872c

Fix track filling on single slider

c8ac5e9

jscheiny added 6 commits April 20, 2018 13:42

Fix border radius bug

7d1aa5a

Add todo

13ccec6

Change lower/upper -> start/end and below/above -> before/after

f7e69ae

Fix range slider

b079378

Example component

daccaf8

Fix lint

5977d41

Refactor default props

191425e

Remove import

2f9d274

giladgray reviewed Apr 24, 2018

View reviewed changes

Address comments

621d193

jscheiny added 2 commits April 26, 2018 17:46

Use utils function

cb10803

Refactor slider tests

813c082

Merge branch 'develop' into js/multi-range-slider

fbfef2c

jscheiny added 2 commits May 1, 2018 16:25

Tests

3549691

Merge branch 'js/multi-range-slider' of github.com:jscheiny/blueprint…

fbb187e

… into review/js/multi-range-slider

jscheiny added 3 commits May 1, 2018 18:30

Fixes for out of order handles

0d12368

Improvements to the example

bbdc8ff

Add default track intent

159c87d

Fix vertical track border radii

8b5f387

Fix lint

0ba20e8

jscheiny mentioned this pull request May 22, 2018

[Core] MultiSlider component #2536

Merged

3 tasks

jscheiny closed this May 22, 2018

jscheiny deleted the js/multi-range-slider branch May 22, 2018 14:58


		export type SliderHandleType = "full" \| "start" \| "end";

		export interface ISliderHandleProps {

MultiRangeSlider component #2407

MultiRangeSlider component #2407

Conversation

jscheiny commented Apr 19, 2018 • edited

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Not addressed in this PR (requires follow-up):

Screenshots

palantirtech commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorca commented Apr 19, 2018

jscheiny commented Apr 19, 2018

jscheiny commented Apr 19, 2018

Use children instead of array of props

jscheiny commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorca commented Apr 19, 2018

jscheiny commented Apr 19, 2018

Fix lint?

llorca commented Apr 19, 2018

jscheiny commented Apr 20, 2018

Merge remote-tracking branch 'upstream/develop' into review/js/multi-range-slider

jscheiny commented Apr 20, 2018

Simplify intent coloring

jscheiny commented Apr 20, 2018

llorca commented Apr 20, 2018

jscheiny commented Apr 20, 2018

jscheiny commented Apr 20, 2018

Example component

giladgray commented Apr 20, 2018

jscheiny commented Apr 24, 2018

Remove import

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscheiny commented Apr 25, 2018

Address comments

jscheiny commented Apr 30, 2018

Refactor slider tests

jscheiny commented May 1, 2018

Merge branch 'develop' into js/multi-range-slider

jscheiny commented May 1, 2018

Merge branch 'js/multi-range-slider' of github.com:jscheiny/blueprint into review/js/multi-range-slider

jscheiny commented May 2, 2018

Add default track intent

jscheiny commented May 2, 2018

Fix vertical track border radii

jscheiny commented May 3, 2018

Fix lint

giladgray commented May 22, 2018

jscheiny commented May 22, 2018

jscheiny commented Apr 19, 2018 •

edited