Skip to content

[EuiDualRange] Draggable highlight area#4776

Merged
thompsongl merged 20 commits intoelastic:masterfrom
thompsongl:4694-range
Jun 14, 2021
Merged

[EuiDualRange] Draggable highlight area#4776
thompsongl merged 20 commits intoelastic:masterfrom
thompsongl:4694-range

Conversation

@thompsongl
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl commented May 3, 2021 •
edited
Loading

Summary

Closes #4694

Opening for early review. I still need to fine tune the dragging and accommodate decimal and non-standard step values (only really works with 1 at present).
Second example in the "Dual range" section has isDraggable enabled.

Adds a draggable highlight area between the two range slider values in EuiDualRange. It is optional and currently off by default. Movement works/feel good for small step values (i.e., a large number of steps in the range), but larger step values (i.e., a small number of steps in the range) makes the drag interaction feel a bit disjointed; looking for thoughts on interaction improvement or suggestions for guidance documentation.

Also moved some global services out of color_picker/utils.ts and into services/

Checklist

  • Revert docs examples
  • Check against all themes for compatibility in both light and dark modes

- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented May 3, 2021 •
edited
Loading

@myasonik @miukimiu Calling on your expertise to help make the right decisions for a11y and any design states that I may not have accounted for. Just high-level things right now while in draft mode.

@nreese Take a look and see if this fits the bill for your time slider.

Note in the summary I've called out some things I'm still working on.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 3, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@nreese
Copy link
Copy Markdown
Contributor

nreese commented May 3, 2021

This is a great addition, thanks for adding this feature. It works just as expected and will make the timeslider some much easier to use.

@elizabetdev
Copy link
Copy Markdown
Contributor

elizabetdev commented May 5, 2021

@thompsongl this new feature is looking great. 🎉

I've been testing other range sliders, and one thing that I noticed is the use of mouse cursors.

Right now, we're using cursor: pointer; to move the thumbs, and according to https://developer.mozilla.org/en-US/docs/Web/CSS/cursor :

The cursor is a pointer that indicates a link. Typically an image of a pointing hand.

So should we be using cursor: ew-resize; instead?

Bidirectional resize cursor.

Is there a better name than isDraggable? Should this be the default?

I like the name, and I don't see why this feature wouldn't be the default. So for me, it should be the default behavior.

What should the active/focus state with keyboard nav look like?

For the focus states, should we first allow to focus all the "group" then focus on each thumb? Calling our expert here, @myasonik.

_focus states range slider@2x

1 - By focus and interacting with the keys left and right, all the group would move one step (both thumbs move one step into the pressed key direction).
2 - Same functionality. Moves one step.
3 - Same functionality. Moves one step.

For the focus/active states design, I wouldn't worry that much. For now, the focus ring and highlights work well. Then I can try to improve them when adding the new Amsterdam styles.

@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented May 5, 2021

Thanks, @miukimiu; this all makes sense to me. I'll try out some cursor change ideas.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 5, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented May 6, 2021 •
edited
Loading

So should we be using cursor: ew-resize; instead?

I looked into this, and I think the answer is no.
First, we'd need to rewrite how this component works to change the cursor just for the thumbs. The thumbs do not actually handle any events; they are pointer-event: none and pass everything through to the underlying <input type=range>.
Second, native HTML <input type=range> do not change the cursor type for the thumb area.
In both cases the thumb is just an extension of the range an does not have standalone behavior.
Perhaps cursor: pointer is the wrong one to use, but we'd need to pick something that makes sense for the range and the thumbs.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 6, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented May 11, 2021

I still need to fine tune the dragging and accommodate decimal and non-standard step values (only really works with 1 at present)

Still looking at this. Two things in particular: 1) decimal (less than 1) step values and 2) step values that as large as the draggable area (the drag distance needs to be reconciled with the distance to the next valid value)

@nreese nreese mentioned this pull request May 14, 2021
Merged
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 26, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@thompsongl thompsongl marked this pull request as ready for review May 27, 2021 16:02
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented May 27, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

elizabetdev
elizabetdev approved these changes May 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thompsongl.

Tested in Chrome, Safari, Edge, and Firefox and works perfectly. 🎉

myasonik
myasonik approved these changes Jun 2, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits but nothing major 🚀

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 3, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 3, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 3, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@elizabetdev elizabetdev mentioned this pull request Jun 8, 2021
Merged
7 tasks
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Jun 14, 2021

Preview documentation changes for this PR: https://eui.elastic.co/pr_4776/

@thompsongl thompsongl merged commit 5e8bec6 into elastic:master Jun 14, 2021
@thompsongl thompsongl mentioned this pull request Jun 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@elizabetdev elizabetdev elizabetdev approved these changes

@myasonik myasonik myasonik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiDualRange] Allow dragging the highlighted value

5 participants

@thompsongl @kibanamachine @nreese @elizabetdev @myasonik