feat(Slider): add option to add Tooltip over thumb by KKoukiou · Pull Request #6624 · patternfly/patternfly-react

KKoukiou · 2021-11-18T16:27:29Z

patternfly-build · 2021-11-18T16:36:28Z

Preview: https://patternfly-react-pr-6624.surge.sh

nicolethoen · 2021-11-19T17:25:37Z

@@ -134,13 +135,22 @@ class ContinuousInput extends React.Component {
    return (
      <>
        <Text component={TextVariants.h3}>Slider value is: {this.state.value.toFixed(2)}</Text>


We should remove this line.

nicolethoen · 2021-11-19T17:30:25Z

+          value={this.state.value}
+          onChange={this.onChange} />
        <br />
        <Text component={TextVariants.h3}>Slider value is: {this.state.valueCustom.toFixed(2)}</Text>


As it is, the Slide value is: text in the example only has 2 values trailing the decimal, but the tooltip has many. I wonder if we should make the number of trailing values something the user can configure, and have it always default to 2? It'd make the aria label much more readable.

Made it default to 2 decimals. Lets add the property for configuring once someone requests it if you 're fine with that.

Could you also remove the trimming in the examples if it's no longer needed?

nicolethoen

Could you also add a unit test which basically verifies that a Tooltip gets rendered whenever thumbHasTooltip is true?

KKoukiou · 2021-11-19T18:10:09Z

Could you also add a unit test which basically verifies that a Tooltip gets rendered whenever thumbHasTooltip is true?

Added

mcarrano

This looks good, although while the tooltip in the example is constrained to 2 decimal places, the value displayed is not. Shouldn't these match? Here's what I'm seeing.

Also, it seems like the Value input examples are broken. Maybe unrelated to your changes, but can you check these?

nicolethoen · 2021-11-23T14:09:51Z

-    return localValue.toString();
+    // For continuous steps default to showing 2 decimals in tooltip
+    // Consider making it configurable via a property
+    return Number(localValue.toFixed(2)).toString();


To piggy pack off of Matt's comment, I think we want to make this same update to the onChangeHandler function. Either that, or in the handleThumbMove function we should trim the continuous value on line 221 to use 2 trailing digits.

KKoukiou · 2021-11-24T11:55:56Z

This looks good, although while the tooltip in the example is constrained to 2 decimal places, the value displayed is not. Shouldn't these match? Here's what I'm seeing.

Also, it seems like the Value input examples are broken. Maybe unrelated to your changes, but can you check these?

This is also broken on master, just checked. Let's fix it in a separate PR as it's unrelated.

For your interest I just checked and it's ed777b2. which broke these.

Added an issue for this bug:
#6644

gabipodolnikova · 2021-11-24T12:46:10Z

-        <Text component={TextVariants.h3}>Slider value is: {this.state.value.toFixed(2)}</Text>
-        <Slider value={this.state.value} onChange={this.onChange} />
+        <Checkbox
+          id="thumb-has-tooltip"


I would add a style={{ marginBottom: 20 }} to the Checkbox.

gabipodolnikova

I also noticed that the Thumb value input example is broken, which it wasn't before. When I move it, I get: TypeError: W.toFixed is not a function Do you see it too?

KKoukiou · 2021-11-24T14:14:10Z

I also noticed that the Thumb value input example is broken, which it wasn't before. When I move it, I get: TypeError: W.toFixed is not a function Do you see it too?

Yes, sorry. I fixed it.

mcarrano

Tooltip option looks good, but some of the Discrete slider examples seem to be broken - specifically the first one and the first two Vale input examples. I cannot drag the slider.

KKoukiou · 2021-11-30T14:23:21Z

Tooltip option looks good, but some of the Discrete slider examples seem to be broken - specifically the first one and the first two Vale input examples. I cannot drag the slider.

@mcarrano This problem exists already upstream, see on the docs pge directly without my PR https://www.patternfly.org/v4/components/slider/

mcarrano

Fair enough @KKoukiou . In that case I will approve this PR, but @nicolethoen @tlabaj do you have any idea when or why these examples broke. We should try to clean them up for the next release. Do we need a new issue for this?

KKoukiou · 2021-12-01T09:42:55Z

@mcarrano I created an issue last week about it: #6644

tlabaj

Looks great! One suggestion.

tlabaj

Looks Good! Thank you!
Looks like you need to update the snapshots and we can merge.

Fixes patternfly#5955

nicolethoen reviewed Nov 19, 2021

View reviewed changes

nicolethoen requested changes Nov 19, 2021

View reviewed changes

KKoukiou force-pushed the slider-tooltip branch from 731fa84 to 637777f Compare November 19, 2021 18:09

KKoukiou force-pushed the slider-tooltip branch from 637777f to 47ec8f7 Compare November 19, 2021 18:24

tlabaj added the ux review label Nov 19, 2021

tlabaj requested a review from mcarrano November 19, 2021 20:07

KKoukiou force-pushed the slider-tooltip branch from 47ec8f7 to 4c7be75 Compare November 22, 2021 10:56

mcarrano suggested changes Nov 22, 2021

View reviewed changes

nicolethoen reviewed Nov 23, 2021

View reviewed changes

nicolethoen requested a review from gabipodolnikova November 23, 2021 15:00

KKoukiou force-pushed the slider-tooltip branch 2 times, most recently from ec1f75e to 34e4351 Compare November 24, 2021 11:59

gabipodolnikova reviewed Nov 24, 2021

View reviewed changes

KKoukiou force-pushed the slider-tooltip branch from 34e4351 to 3f7e75a Compare November 24, 2021 12:51

gabipodolnikova suggested changes Nov 24, 2021

View reviewed changes

KKoukiou force-pushed the slider-tooltip branch from 3f7e75a to e2ddb90 Compare November 24, 2021 14:13

KKoukiou force-pushed the slider-tooltip branch from e2ddb90 to 4ec0bca Compare November 24, 2021 14:37

gabipodolnikova approved these changes Nov 24, 2021

View reviewed changes

nicolethoen approved these changes Nov 30, 2021

View reviewed changes

nicolethoen requested a review from mcarrano November 30, 2021 13:46

mcarrano suggested changes Nov 30, 2021

View reviewed changes

mcarrano approved these changes Nov 30, 2021

View reviewed changes

tlabaj requested changes Dec 1, 2021

View reviewed changes

Comment thread packages/react-core/src/components/Slider/Slider.tsx Outdated

KKoukiou force-pushed the slider-tooltip branch from 4ec0bca to 27932c6 Compare January 4, 2022 08:30

tlabaj approved these changes Jan 5, 2022

View reviewed changes

feat(Slider): add option to add Tooltip over thumb

e9d2d2f

Fixes patternfly#5955

KKoukiou force-pushed the slider-tooltip branch from 27932c6 to e9d2d2f Compare January 6, 2022 08:01

tlabaj merged commit cbe60e6 into patternfly:main Jan 6, 2022

Conversation

KKoukiou commented Nov 18, 2021

Uh oh!

patternfly-build commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

KKoukiou commented Nov 19, 2021

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KKoukiou commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabipodolnikova left a comment

Choose a reason for hiding this comment

Uh oh!

KKoukiou commented Nov 24, 2021

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

KKoukiou commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

KKoukiou commented Dec 1, 2021

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

patternfly-build commented Nov 18, 2021 •

edited

Loading

KKoukiou commented Nov 24, 2021 •

edited

Loading

KKoukiou commented Nov 30, 2021 •

edited

Loading