Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiDualRange] Draggable highlight area #4776

Merged
merged 20 commits into from
Jun 14, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented May 3, 2021

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
Contributor Author

thompsongl commented May 3, 2021

@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

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

@nreese
Copy link
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
Contributor

@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
Contributor Author

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

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

thompsongl commented May 6, 2021

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

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

@thompsongl
Copy link
Contributor Author

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)

@kibanamachine
Copy link

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

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

Copy link
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. 🎉

Copy link
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 🚀

src-docs/src/views/range/dual_range.js Show resolved Hide resolved
src-docs/src/views/range/dual_range.js Outdated Show resolved Hide resolved
src-docs/src/views/range/dual_range.js Outdated Show resolved Hide resolved
src/components/form/range/dual_range.tsx Outdated Show resolved Hide resolved
src/components/form/range/dual_range.tsx Outdated Show resolved Hide resolved
src/components/form/range/dual_range.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit 5e8bec6 into elastic:master Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiDualRange] Allow dragging the highlighted value
5 participants