[Slider] Use percentage values in slider styles #2341

reiv · 2018-03-30T17:48:09Z

Fixes #2338

Changes proposed in this pull request:

This PR changes the slider styles to use percentages instead of absolute pixel values. This should allow the slider to be freely resized without requiring a forced re-render.

Screenshot

This recording shows the slider being resized (width is being set in the inspector).

llorca · 2018-03-30T17:56:15Z

uh oh, that's great stuff. seems to be working well!

I'm seeing a little regression with the 3rd docs example: the slider handle is gone

reiv · 2018-03-30T18:10:42Z

Restore PEMDAS

Preview: documentation | landing | table

giladgray · 2018-04-03T17:25:58Z

@reiv dude yes this is brilliant! do we even need tickSize (pixels) anymore?

reiv · 2018-04-03T18:45:41Z

@giladgray Had to leave that in because there was some stuff in the slider handle code (clientToValue) that depended on it:

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

Lines 93 to 94 in 66bf867

    
           // convert pixels to range value in increments of `stepSize` 
        
           const valueDelta = Math.round(pixelDelta / (tickSize * stepSize)) * stepSize;

Perhaps that can be rewritten too, but I don't fully understand it yet.

reiv · 2018-04-04T18:35:33Z

Unrelated to this issue, but it looks like there's a possible oversight in Handle.render():

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

Lines 56 to 58 in 6de4f5a

    
           const { handleMidpoint } = this.getHandleMidpointAndOffset(this.handleElement, true); 
        
           const offset = Math.round((value - min) * tickSize - handleMidpoint); 
        
           const style: React.CSSProperties = vertical ? { bottom: offset } : { left: offset };

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

Line 199 in 6de4f5a

    
           private getHandleMidpointAndOffset(handleElement: HTMLElement, useOppositeDimension = false) {

I'm assuming getHandleMidpointAndOffset has a useOppositeDimension param to support getting the other midpoint of non-square handles (as it stands, both return 8, i.e. half of the handle's 16px side length). But in this case, we don't want the opposite dimension -- we'd get the vertical handle midpoint on a horizontal slider, resulting in this (handle height changed in inspector to exaggerate):

Was there ever a plan to permit controlling the handle size? Anyway, useOppositeDimension isn't going to affect anything as long as the handle is a square, but I'm going to go ahead and change this here just in case.

Edit: Hmm, looks like I misunderstood, and it was there for a very different reason: the range slider handle is special in that its midpoint is actually on the visible edge of the handle. But getHandleMidpointAndOffset doesn't take into account the 8px margin that's used to offset the midpoint, so measuring the other side happens to give the correct midpoint instead. I think this is still better than the alternative (using getComputedStyle() to include the margin); Going to add a comment for posterity though.

reiv · 2018-04-04T20:28:36Z

Restore previous logic with added comment

Preview: documentation | landing | table

giladgray

@reiv this looks so great!! i'll look into removing tickSize after merging this. just one thought about numeric precision.

giladgray · 2018-04-05T17:39:15Z

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

-        const offset = Math.round((value - min) * tickSize - handleMidpoint);
-        const style: React.CSSProperties = vertical ? { bottom: offset } : { left: offset };
+        const offsetRatio = (value - min) * tickSizeRatio;
+        const offsetCalc = `calc(${offsetRatio * 100}% - ${handleMidpoint}px)`;


giladgray · 2018-04-05T17:41:09Z

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

        const initialValue = clamp(this.props.initialValue, this.props.min, this.props.max);

-        let offset = Math.round((initialValue - this.props.min) * tickSize);
-        let size = Math.round((this.props.value - initialValue) * tickSize);


it's cool that we don't need to round anymore, but i think we want to ensure a maximum number of decimal places via number.toFixed(2) to reduce sub-pixel rendering.

Do you mean rounding the value of offsetRatio? I don't think that would accomplish much because the actual computed value in pixels doesn't have any restrictions on the number of decimal places. Here's an example of a round percentage yielding sub-pixel precision (shows inline styles and computed value from inspector):

Unfortunately CSS calc provides no facilities for rounding the result. 😢

no i mean ${(offset * 100).toFixed(2)}%

sub-pixel renders are going to happen here for sure, but there's a limit to precision on the screen. it's definitely possible that my suggestion isn't really helpful?

Well, at the very least we'll get inline styles that are easy on the eyes if anyone happens to be snooping around with inspect element. I'll make it happen 👍

reiv · 2018-04-05T18:18:33Z

@giladgray I believe tickSize is still needed because the handle code needs to know about the width of its container, for two reasons: to achieve "snapping" of the handle's offset to the correct tick position, and to convert the move events' offsets (as a ratio of the slider size) back to the represented values.

reiv · 2018-04-05T18:41:28Z

Renames

Preview: documentation | landing | table

reiv · 2018-04-05T18:56:42Z

Fix test

Preview: documentation | landing | table

giladgray · 2018-04-05T21:23:52Z

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

-        const offsetPercentage = `${offset * 100}%`;
-        const sizePercentage = `${size * 100}%`;
+        const offsetPercentage = `${(offset * 100).toFixed(2)}%`;
+        const sizePercentage = `${(size * 100).toFixed(2)}%`;


ok well now we should definitely have a formatPercentage(size) utility function!

it could go in coreSlider.tsx

giladgray · 2018-04-05T21:24:08Z

packages/core/test/slider/sliderTests.tsx

@@ -193,7 +193,7 @@ describe("<Slider>", () => {
        const style = renderSlider(<Slider initialValue={-10} min={0} value={5} />)
            .find("." + Classes.SLIDER_PROGRESS)
            .prop("style") as React.CSSProperties;
-        assert.strictEqual(style.left, "0%");
+        assert.strictEqual(style.left, "0.00%");


yeah... always exactly 2 decimals.

reiv · 2018-04-05T22:41:16Z

Add formatPercentage helper function

Preview: documentation | landing | table

reiv · 2018-04-05T22:41:21Z

Renames

Preview: documentation | landing | table

giladgray

hooray!

reiv added 2 commits March 30, 2018 19:28

Rewrite Slider geometry to use percentage values

c5ad437

Update test

c2f70b2

llorca requested a review from giladgray March 30, 2018 17:54

Remove stray comments

d7d83e8

Restore PEMDAS

016e981

reiv added 2 commits April 4, 2018 20:37

Don't set useOppositeDimension flag to get handle offset

c6abc66

Restore previous logic with added comment

be1b9e7

giladgray reviewed Apr 5, 2018

View reviewed changes

reiv added 2 commits April 5, 2018 20:33

Limit precision of percentages to two decimal places

312e764

Renames

422b334

Fix test

b5714c9

giladgray suggested changes Apr 5, 2018

View reviewed changes

reiv added 2 commits April 6, 2018 00:30

Add formatPercentage helper function

ff8de2a

Renames

a358270

giladgray approved these changes Apr 5, 2018

View reviewed changes

giladgray merged commit f6562f5 into palantir:develop Apr 5, 2018

reiv deleted the rv/resizable-sider branch April 5, 2018 22:47

giladgray mentioned this pull request Apr 19, 2018

Prepare release v2.2.0 #2411

Merged

snyk-bot mentioned this pull request Jan 16, 2022

[Snyk] Security upgrade marked from 1.2.9 to 4.0.10 majacQ/blueprint#67

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider] Use percentage values in slider styles #2341

[Slider] Use percentage values in slider styles #2341

reiv commented Mar 30, 2018

llorca commented Mar 30, 2018

reiv commented Mar 30, 2018

giladgray commented Apr 3, 2018

reiv commented Apr 3, 2018 •

edited

Loading

reiv commented Apr 4, 2018 •

edited

Loading

reiv commented Apr 4, 2018

giladgray left a comment

giladgray Apr 5, 2018

giladgray Apr 5, 2018

reiv Apr 5, 2018

giladgray Apr 5, 2018

giladgray Apr 5, 2018

reiv Apr 5, 2018

reiv commented Apr 5, 2018

reiv commented Apr 5, 2018

reiv commented Apr 5, 2018

giladgray Apr 5, 2018

giladgray Apr 5, 2018

reiv Apr 5, 2018

giladgray Apr 5, 2018

reiv commented Apr 5, 2018

reiv commented Apr 5, 2018

giladgray left a comment

[Slider] Use percentage values in slider styles #2341

[Slider] Use percentage values in slider styles #2341

Conversation

reiv commented Mar 30, 2018

Fixes #2338

Changes proposed in this pull request:

Screenshot

llorca commented Mar 30, 2018

reiv commented Mar 30, 2018

Restore PEMDAS

giladgray commented Apr 3, 2018

reiv commented Apr 3, 2018 • edited Loading

reiv commented Apr 4, 2018 • edited Loading

reiv commented Apr 4, 2018

Restore previous logic with added comment

giladgray left a 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

reiv commented Apr 5, 2018

reiv commented Apr 5, 2018

Renames

reiv commented Apr 5, 2018

Fix test

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

reiv commented Apr 5, 2018

Add formatPercentage helper function

reiv commented Apr 5, 2018

Renames

giladgray left a comment

Choose a reason for hiding this comment

reiv commented Apr 3, 2018 •

edited

Loading

reiv commented Apr 4, 2018 •

edited

Loading