feat(Slider): Added new slider component #5358

tlabaj · 2021-01-22T05:28:46Z

What: Closes #5187

TODO: integration tests

patternfly-build · 2021-01-22T05:33:55Z

PF4 preview: https://patternfly-react-pr-5358.surge.sh
A11y report: https://patternfly-react-pr-5358-a11y.surge.sh

codecov-io · 2021-01-22T05:45:27Z

Codecov Report

Merging #5358 (12b2d7e) into master (4f199c4) will decrease coverage by 0.09%.
The diff coverage is 44.18%.

@@            Coverage Diff             @@
##           master    #5358      +/-   ##
==========================================
- Coverage   52.24%   52.15%   -0.10%     
==========================================
  Files         575      577       +2     
  Lines       11044    11173     +129     
  Branches     4116     4162      +46     
==========================================
+ Hits         5770     5827      +57     
- Misses       4538     4609      +71     
- Partials      736      737       +1

Flag	Coverage Δ
patternfly4	`52.15% <44.18%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ckages/react-core/src/components/Slider/Slider.tsx	`41.32% <41.32%> (ø)`
...es/react-core/src/components/Slider/SliderStep.tsx	`87.50% <87.50%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24739d0...12b2d7e. Read the comment docs.

KKoukiou

Thanks for working on this, we look forward to this feature in cockpit.
I quickly tried it out, I have some pointers for improvement.

Keyboard interactions don't work
Clicking on random place in the slider should move the current position there

Tried it on firefox-83.0-13.fc33.x86_64.

mcarrano

This is looking good @tlabaj . Just had a couple of issues/questions.

For the continuous slider I notice that it never returns fractional values. Should we allow the consumer to set precision via a prop? There might be cases where you don't want the returned value always rounded to a whole number.
For the "Thumb value input" example, I was able to get the thumb and value input to disappear from view by entering a value beyond the upper limit of the slider. Seems like there should be some value checking to make sure the slider value can never exceed the upper or lower limits.
The keyboard interaction worked for me except on the last example of the slider with actions. @KKoukiou how were you expecting the interaction to work. We specified that the slider value should only change upon pressing Enter or tabbing out of the field. Not on every keystroke.
Agree with @KKoukiou that you should also be able to update the value by clicking on the slider bar.

KKoukiou · 2021-01-22T14:51:16Z

This is looking good @tlabaj . Just had a couple of issues/questions.

* For the continuous slider I notice that it never returns fractional values. Should we allow the consumer to set precision via a prop? There might be cases where you don't want the returned value always rounded to a whole number.

* For the "Thumb value input" example, I was able to get the thumb and value input to disappear from view by entering a value beyond the upper limit of the slider. Seems like there should be some value checking to make sure the slider value can never exceed the upper or lower limits.

* The keyboard interaction worked for me except on the last example of the slider with actions. @KKoukiou how were you expecting the interaction to work. We specified that the slider value should only change upon pressing Enter or tabbing out of the field. Not on every keystroke.

I expect that clicking on the circle of the slider and then using my right and left keyboard keys, should move the slider value left and right accordingly. This does not happen for me at least.
I just tried on all browsers i have available, firefox, chrome, edge and epiphany, its' the same everywhere.
Let me know if you still can't reproduce it.

* Agree with @KKoukiou that you should also be able to update the value by clicking on the slider bar.

tlabaj · 2021-01-22T14:57:58Z

Thanks @KKoukiou and @mcarrano You are right, you have to tab into the thumb to be able to move it. I will work on that update and clicking on the slider to update the value.

tlabaj · 2021-01-22T17:49:52Z

@mcarrano

This is looking good @tlabaj . Just had a couple of issues/questions.

For the continuous slider I notice that it never returns fractional values. Should we allow the consumer to set precision via a prop? There might be cases where you don't want the returned value always rounded to a whole number.

It does return the fractional value. The demo code takes the floor and displays that. I can update that if you like.

The keyboard interaction worked for me except on the last example of the slider with actions. @KKoukiou how were you expecting the interaction to work. We specified that the slider value should only change upon pressing Enter or tabbing out of the field. Not on every keystroke.

For this one, are you talking about both the sliders in the action example or the very last one. I could not reproduce an issue with the keyboard interaction other than what was mentioned about clicking on the slider and clicking the thumb to apply focus.

jessiehuff

Really nice job with this so far, Titani!!! 🎉 ⭐ From my perspective, I think the keyboard interaction looks pretty good so far, and I think handling the aria position with the aria-valuetext was a decent way to do it.

The only thing I've noticed that I thought was a little odd is that I can't seem to get the slider to 0 for a lot of the examples by keyboard, though it looks like this is true by mouse as well for some. Is it intentional to enforce a minimum value like that? Wondering if some of them may be a tad odd though. For example, if you're looking at the discrete example and the slider thumb starts at 2 and you drag it to 0, the slider value above it stays at 2 (I noticed this is true if it starts at any number and you drag it to 0, it will always stay at the previous number).

mcarrano · 2021-01-22T18:54:57Z

For this one, are you talking about both the sliders in the action example or the very last one. I could not reproduce an issue with the keyboard interaction other than what was mentioned about clicking on the slider and clicking the thumb to apply focus.

Yes, I'm talking about the very last one. If I change the value in the number field and hit Enter or tab out of the field, nothing happens. I'm having the same problem with the second example under Value input.

jessiehuff

LGTM! Looks like you fixed the thing I was seeing before. 🙂

mcoker · 2021-01-22T22:59:18Z

packages/react-core/src/components/Slider/Slider.tsx

+        onFocus={onInputFocus}
+        onBlur={onBlur}
+      />
+      {inputLabel && <span className={css(styles.inputGroupText, 'pf-m-plain')}>{inputLabel}</span>}


Technically an <InputGroup> is only needed here if this text exists, but it doesn't hurt to leave it the way it is.

Also curious why you don't use <InputGroupText> for this element? Looks like we would need an enhancement for isPlain or something to apply .pf-m-plain.

I can open a separate issue for that enhancement if we don't want to do it here. It was added as part of patternfly/patternfly#3711

I have updated this to use iInputGroup, InputGroupText and TextInput. It was an oversight.

The InputGroupText component still need to be updated for the plain variation. @mcarrano do you want me to open a new issue to make that update? The style is still applied inline here so it still displays correctly.

Sure. Go ahead and open a core issue @tlabaj .

@mcarrano it is a react issue. Core already made the update.
here is the new issue
#5367

mcoker · 2021-01-22T22:59:30Z

packages/react-core/src/components/Slider/Slider.tsx

+  };
+
+  const inputGroup = (
+    <div className={css(styles.inputGroup)}>


I'm curious why you don't use <InputGroup> here?

mcoker · 2021-01-22T22:59:51Z

packages/react-core/src/components/Slider/Slider.tsx

+
+  const inputGroup = (
+    <div className={css(styles.inputGroup)}>
+      <input


Curious why you don't use <TextInput>?

mcoker

LGTM! Left some questions, but they shouldn't hold up merging this PR.

mcarrano

Thanks for making those updates @tlabaj . Looks good to me!

kmcfaul · 2021-01-25T16:13:57Z

Looks good, just two interaction questions.

In the Thumb input value example, you can move the slider but the text input doesn't update with the current slider value. The other slider examples do update so I'm not sure if this is intentional.

In the Actions example, you can "lock" the slider but only the text input is disabled, you can still drag the slider and change the value that way. Is the lock meant only for the text input?

tlabaj · 2021-01-25T16:29:12Z

@kmcfaul

Looks good, just two interaction questions.

In the Thumb input value example, you can move the slider but the text input doesn't update with the current slider value. The other slider examples do update so I'm not sure if this is intentional.

I see the issue.

In the Actions example, you can "lock" the slider but only the text input is disabled, you can still drag the slider and change the value that way. Is the lock meant only for the text input?
@mcarrano @mcoker there is no styling to disable a slider correct?

mcarrano · 2021-01-25T17:10:28Z

In the Actions example, you can "lock" the slider but only the text input is disabled, you can still drag the slider and change the value that way. Is the lock meant only for the text input?
@mcarrano @mcoker there is no styling to disable a slider correct?

Right. There is no disabled styling for the Slider. We could add that but I'm hesitant as there is not a product use case for it. Can you just disable events on the slider for now @tlabaj to make this example work better?

tlabaj · 2021-01-25T17:18:38Z

In the Actions example, you can "lock" the slider but only the text input is disabled, you can still drag the slider and change the value that way. Is the lock meant only for the text input?
@mcarrano @mcoker there is no styling to disable a slider correct?

Right. There is no disabled styling for the Slider. We could add that but I'm hesitant as there is not a product use case for it. Can you just disable events on the slider for now @tlabaj to make this example work better?

@mcarrano I could. However, I don't think that is a good idea. I think that could be confusing to the consumer since there is no visual indication that the slider is disabled.

mcarrano · 2021-01-25T17:26:22Z

@tlabaj @kmcfaul Do you think it's better to leave as is or to remove the example? Again, I see this as a "nice to have" feature that we don't have a hard requirement for right now.

kmcfaul

I think I'd be fine with it either way. Since this will be a beta component we can update the example later if we leave it in.

jessiehuff

So I only see this on the actions example so it might just be that example's issue, but I noticed that when I try to navigate the actions example, it doesn't quite behave right (tring to use the right arrow key on the thumb doesn't work, only the left) and when I try to use the increase/plus button, it adds on a bizarre increment:

nicolethoen · 2021-01-25T19:01:12Z

packages/react-core/src/components/Slider/Slider.tsx

+  currentValue?: number;
+  /** Flag indicating if the slider is is discrete */
+  isDiscrete?: boolean;
+  /** Array of slider step objects (value and label of each step) for the slider. If this is provided, the numSteps prop will be ignored.*/


is there still a numSteps prop? I dont see it.

nope. that was removed

nicolethoen · 2021-01-25T19:03:21Z

we should also open an issue to make the sliders work on mobile.

jessiehuff

LGTM on the accessibility side! 🙂

tlabaj · 2021-01-25T19:08:34Z

@nicolethoen there is already an issue open for that. #5363

patternfly-build · 2021-01-25T20:03:23Z

Your changes have been released in:

@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]

Thanks for your contribution! 🎉

tlabaj added ux review A11y labels Jan 22, 2021

tlabaj requested review from jschuler, mcarrano, jessiehuff, mcoker and kmcfaul January 22, 2021 05:28

tlabaj assigned kmcfaul Jan 22, 2021

KKoukiou reviewed Jan 22, 2021

View reviewed changes

mcarrano requested changes Jan 22, 2021

View reviewed changes

tlabaj force-pushed the slider branch from e953b52 to e4c3c6e Compare January 22, 2021 18:13

jessiehuff reviewed Jan 22, 2021

View reviewed changes

jessiehuff previously approved these changes Jan 22, 2021

View reviewed changes

mcoker reviewed Jan 22, 2021

View reviewed changes

mcoker previously approved these changes Jan 22, 2021

View reviewed changes

tlabaj dismissed stale reviews from mcoker and jessiehuff via b3b832d January 22, 2021 23:37

mcarrano previously approved these changes Jan 23, 2021

View reviewed changes

tlabaj dismissed mcarrano’s stale review via ad798b5 January 25, 2021 15:21

Titani added 2 commits January 25, 2021 10:28

feat(Slider): Added new slider component

e90e075

update slider so that thumb is focused on click

62318a6

Titani added 6 commits January 25, 2021 10:28

updated slider to update on click

630b638

fix linting

2d6a63c

fix demos and keyboard interaction issues

d300b3b

update demo to only show 2 decimal points

b958f8f

update examples so discrete slider inputs snap to closest value

60d0491

Updates from review comments

66d940f

tlabaj force-pushed the slider branch from ad798b5 to 66d940f Compare January 25, 2021 15:29

revert snap change from drawer conflict

12b2d7e

fix broken thumb exampl and on;yt show 2 decimal places inputs

ec0fbf2

kmcfaul previously approved these changes Jan 25, 2021

View reviewed changes

jessiehuff reviewed Jan 25, 2021

View reviewed changes

fix example

e98b3d9

tlabaj dismissed kmcfaul’s stale review via e98b3d9 January 25, 2021 18:38

nicolethoen reviewed Jan 25, 2021

View reviewed changes

jessiehuff previously approved these changes Jan 25, 2021

View reviewed changes

fix comment

240cb29

tlabaj dismissed jessiehuff’s stale review via 240cb29 January 25, 2021 19:10

nicolethoen approved these changes Jan 25, 2021

View reviewed changes

kmcfaul approved these changes Jan 25, 2021

View reviewed changes

kmcfaul merged commit 90df385 into patternfly:master Jan 25, 2021

marshmalien mentioned this pull request Feb 3, 2021

Update Instance Group Form to use Slider when available ansible/awx#7777

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Slider): Added new slider component #5358

feat(Slider): Added new slider component #5358

tlabaj commented Jan 22, 2021

patternfly-build commented Jan 22, 2021 •

edited

Loading

codecov-io commented Jan 22, 2021 •

edited

Loading

KKoukiou left a comment

mcarrano left a comment

KKoukiou commented Jan 22, 2021

tlabaj commented Jan 22, 2021

tlabaj commented Jan 22, 2021

jessiehuff left a comment

mcarrano commented Jan 22, 2021

jessiehuff left a comment

mcoker Jan 22, 2021

mcoker Jan 22, 2021

tlabaj Jan 25, 2021

mcarrano Jan 25, 2021

tlabaj Jan 25, 2021

mcoker Jan 22, 2021

mcoker Jan 22, 2021

mcoker left a comment

mcarrano left a comment

kmcfaul commented Jan 25, 2021

tlabaj commented Jan 25, 2021 •

edited

Loading

mcarrano commented Jan 25, 2021

tlabaj commented Jan 25, 2021

mcarrano commented Jan 25, 2021

kmcfaul left a comment

jessiehuff left a comment

nicolethoen Jan 25, 2021

tlabaj Jan 25, 2021

nicolethoen commented Jan 25, 2021

jessiehuff left a comment

tlabaj commented Jan 25, 2021 •

edited

Loading

patternfly-build commented Jan 25, 2021

feat(Slider): Added new slider component #5358

feat(Slider): Added new slider component #5358

Conversation

tlabaj commented Jan 22, 2021

patternfly-build commented Jan 22, 2021 • edited Loading

codecov-io commented Jan 22, 2021 • edited Loading

Codecov Report

KKoukiou left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

KKoukiou commented Jan 22, 2021

tlabaj commented Jan 22, 2021

tlabaj commented Jan 22, 2021

jessiehuff left a comment

Choose a reason for hiding this comment

mcarrano commented Jan 22, 2021

jessiehuff 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

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

kmcfaul commented Jan 25, 2021

tlabaj commented Jan 25, 2021 • edited Loading

mcarrano commented Jan 25, 2021

tlabaj commented Jan 25, 2021

mcarrano commented Jan 25, 2021

kmcfaul left a comment

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolethoen commented Jan 25, 2021

jessiehuff left a comment

Choose a reason for hiding this comment

tlabaj commented Jan 25, 2021 • edited Loading

patternfly-build commented Jan 25, 2021

patternfly-build commented Jan 22, 2021 •

edited

Loading

codecov-io commented Jan 22, 2021 •

edited

Loading

tlabaj commented Jan 25, 2021 •

edited

Loading

tlabaj commented Jan 25, 2021 •

edited

Loading