EuiDualRange component #1485

thompsongl · 2019-01-28T16:47:43Z

Summary

Resolves #496

Refactor EuiRange into smaller, shareable components (no breaking changes unless you count class names)
New EuiDualRange that uses EuiRange[X] components
Move Range docs/examples out of form controls section

Checklist

~~- [ ] This required updates to Framer X components~~

thompsongl · 2019-01-28T18:13:13Z

@nreese @thomasneirynck Let me know your thoughts on the API, specifically the returned value array for the 2 values on onChange. The actual input element will also have the 2 values reflected in its value attribute, but it'll appear as comma separated (we're "extending" the native behavior of the underlying form element here)

thomasneirynck · 2019-01-28T18:41:25Z

@thompsongl love it!

Array with 2 numbers works for me.

As for the input-element having the value, that's fine imho. That is also probably something we do not really have to worry about from a user perspective, correct? (ie. that the dual-range component is implemented using <input> seems only really relevant for testing).

thompsongl · 2019-01-28T18:47:28Z

@thomasneirynck Great!
Yes, the use of input is mostly an implementation detail, but I didn't want to alienate folks who rely heavily on native form events (e.g., submit). I think in most cases the React prop value will be consumed

cchaos · 2019-01-28T19:47:28Z

@thompsongl Let me know when you've been able to

Incorporate changes from #1461

Then I can do a more thorough review.

At first glance (not at the code) I just saw a couple of issues:

I think the dual range should always showRange

The width of the minimum input resizes when dragging the handles

thompsongl · 2019-01-28T20:49:49Z

@cchaos will do.

Good call on showRange. I'll default to true there.
This happens because the max value for the lower range value is the upper range value. There was a maxWidth calculation in there from EuiRange based on the number of digits in max. When the upper range value changes from 3 digits to 4 digits, the size changes. To avoid the resize, we'd probably be looking at erring on the side of larger inputs. Does that work?

cchaos · 2019-01-28T21:11:33Z

Yes, that's correct. Whatever is the largest possible length of the value – meaning the most amount of characters.

nreese · 2019-01-29T15:09:17Z

@thompsongl Looks really nice. Need some error handling for when showInputs is true that does not show the range, or range handles when max is less then min

Since this PR refactors EuiRange so much. Should I just close #1461 and let you migrate those ideas in this PR?

thompsongl · 2019-01-29T15:20:40Z

@nreese Yeah, I have a task in here to incorporate changes from that PR. I'm fine pulling those changes into this changeset if #1461 won't merge soon. I'm also fine dealing with the merge conflicts if we want the commit history from your work.

thompsongl · 2019-01-30T17:05:53Z

@nreese @chandlerprall @cchaos I've incorporated the concepts from #1461 and addressed all other feedback.

Re: Validation:

The range/highlight track is now hidden for invalid values
Tooltip is conditionally hidden for empty string value
isInvalid is passed to parent components via onChange (implementation differs between range and dualRange because of reliance on event)

cchaos · 2019-01-30T19:54:16Z

@thompsongl

The other thing that #1461 did was to allow the inputs to be completely empty. Right now when you delete the contents it always reverts to 0 and doesn't allow the user to start typing without a beginning 0 unless they move the cursor.

Your PR:

His PR:

I will make a note to myself that we need to flesh out the docs and break apart the examples for this component. But that can come later so that we can unblock the Kibana implementation.

src/components/form/range/_dual_range.scss

cchaos · 2019-01-30T19:57:03Z

src/components/form/range/_range_highlight.scss

+
+  &__progress {
+    height: 4px;
+    border-radius: 4px;


Note to @snide : I'll do a follow-up PR to clean up PX values. Not sure when/why they were never variables to begin with.

src/components/form/range/dual_range.js

src/services/number/number.test.tsx

… docs

thompsongl · 2019-02-01T19:47:13Z

@chandlerprall I've fixed the first 3 of your comments. Good catch on that front!

I'm working through options for the last one.

thompsongl · 2019-02-01T20:40:08Z

Updates for the last comment from @chandlerprall

chandlerprall · 2019-02-05T17:56:47Z

@snide @cchaos thoughts on the modified classes? e.g. https://github.com/elastic/eui/pull/1485/files#diff-7ff6102775c5757cab5eb3e9443c3bff

cchaos · 2019-02-05T18:59:43Z

Hmm, there are some class changes that look concerning especially in the jest files, though I thought they looked right when I reviewed (like compressed). I'll have to take another look.

chandlerprall

Just a couple small things. Nice refactoring though!!

src/components/form/range/index.d.ts

src/services/number/number.ts

src/components/form/range/dual_range.js

src/services/number/number.ts

chandlerprall

LGTM; reviewed code changes and tested UX. Going to hold off on approval until @cchaos takes a second look at those classnames.

thompsongl · 2019-02-05T21:16:03Z

Re: class name changes:
I should've put a comment in there from the start, but it was a Zoom conversation with @snide that had me believe that class name changes (typically) do no constitute breaking changes. After the refactor, it was clear that there were a lot more BEM Block-level components than the Sass showed originally. React component == Sass Block was my thinking.

Could be that this isn't your concern, but more context for you all, nonetheless.

src/components/form/range/range_slider.js

cchaos

LGTM. Just had a couple questions about multiple components in one file.

src/components/form/range/range_track.js

src/components/form/range/dual_range.js

@cchaos

Name change suggestion from @cchaos Co-Authored-By: thompsongl <[email protected]>

cchaos

Great, Thank you! Sorry it took so long. I know others will be excited for this one.

thompsongl added 9 commits January 24, 2019 16:39

WIP: range component break down; dualRange inital

6146f3d

WIP: dual range docs examples

b781aee

WIP: propTypes; type def updates; baseline keyboard nav

b5fabbc

WIP: hasFocus for range highlight

9cbe793

WIP: better rangeSlider ref access

6b6acbd

WIP: sass refactor using component boundaries

e859174

update euiRange tests; create euiDualRange tests

1fbd0b5

move range examples out of formControl; clean up

a6eb6e7

remove thumb focus when disabled

90ff293

thompsongl requested review from cchaos and chandlerprall January 28, 2019 18:09

update docs to share intro

e3672ea

default showRange to true for EuiDualRange; prop clean up

e7ade55

thompsongl added 5 commits January 29, 2019 09:33

class-based focus style when input is not focusable

0f162bb

size input based on max number of characters via digits prop

b4e3dc2

pass aria attrs through to focusable, changeable elements

2c1954a

number service: isWithinRange

a404ee7

account for invalid states; pass isValid to parent via onChange

a742311

cchaos reviewed Jan 30, 2019

View reviewed changes

number service tests, scss whitespace clean up, onChange fn signature…

a3dbc93

… docs

better thumb swap logic; docs notice about value retrieval

774935e

thompsongl added 2 commits February 1, 2019 14:44

Merge branch 'master' into 496-dual-range

5dc15bf

Merge branch 'master' into 496-dual-range

0a17e9f

alexfrancoeur mentioned this pull request Feb 4, 2019

[Maps] Use EUI dual range component for layer definition elastic/kibana#29994

Closed

chandlerprall suggested changes Feb 5, 2019

View reviewed changes

src/components/form/range/index.d.ts Outdated Show resolved Hide resolved

src/components/form/range/index.d.ts Outdated Show resolved Hide resolved

src/services/number/number.ts Outdated Show resolved Hide resolved

src/components/form/range/dual_range.js Outdated Show resolved Hide resolved

thompsongl added 2 commits February 5, 2019 13:27

Merge branch 'master' into 496-dual-range

79b84d1

ts fixes; isWithinRange simplification

b9817a0

chandlerprall suggested changes Feb 5, 2019

View reviewed changes

src/services/number/number.ts Outdated Show resolved Hide resolved

min and max 0-length check

3051f73

chandlerprall reviewed Feb 5, 2019

View reviewed changes

cchaos reviewed Feb 5, 2019

View reviewed changes

src/components/form/range/range_slider.js Show resolved Hide resolved

Merge branch 'master' into 496-dual-range

5d3cd63

cchaos approved these changes Feb 6, 2019

View reviewed changes

cchaos and others added 3 commits February 6, 2019 10:20

rangeClasses -> sliderClasses

4156dc3

Name change suggestion from @cchaos Co-Authored-By: thompsongl <[email protected]>

move all components to individual files

6712617

Merge branch 'master' into 496-dual-range

1d8e323

cchaos approved these changes Feb 6, 2019

View reviewed changes

chandlerprall approved these changes Feb 6, 2019

View reviewed changes

thompsongl merged commit d491d31 into elastic:master Feb 6, 2019

thompsongl mentioned this pull request Feb 11, 2019

EuiRange show empty string in input when value is empty #1458

Closed

chandlerprall mentioned this pull request Feb 15, 2019

Upgrade to EUI 7.1.0 elastic/kibana#31304

Merged

thompsongl mentioned this pull request Feb 20, 2019

EuiRange and EuiDualRange fixes #1580

Merged

12 tasks

thomasneirynck mentioned this pull request Mar 1, 2019

[Maps] Use dual range component elastic/kibana#32273

Merged

cchaos mentioned this pull request May 8, 2019

Allow custom values for EuiRange #960

Closed

snide added the typescript label Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EuiDualRange component #1485

EuiDualRange component #1485

thompsongl commented Jan 28, 2019 •

edited

Loading

thompsongl commented Jan 28, 2019 •

edited

Loading

thomasneirynck commented Jan 28, 2019

thompsongl commented Jan 28, 2019

cchaos commented Jan 28, 2019 •

edited

Loading

thompsongl commented Jan 28, 2019

cchaos commented Jan 28, 2019

nreese commented Jan 29, 2019

thompsongl commented Jan 29, 2019

thompsongl commented Jan 30, 2019 •

edited

Loading

cchaos commented Jan 30, 2019

cchaos Jan 30, 2019

thompsongl commented Feb 1, 2019

thompsongl commented Feb 1, 2019

chandlerprall commented Feb 5, 2019

cchaos commented Feb 5, 2019

chandlerprall left a comment

chandlerprall left a comment

thompsongl commented Feb 5, 2019

cchaos left a comment

cchaos left a comment

EuiDualRange component #1485

EuiDualRange component #1485

Conversation

thompsongl commented Jan 28, 2019 • edited Loading

Summary

Checklist

thompsongl commented Jan 28, 2019 • edited Loading

thomasneirynck commented Jan 28, 2019

thompsongl commented Jan 28, 2019

cchaos commented Jan 28, 2019 • edited Loading

thompsongl commented Jan 28, 2019

cchaos commented Jan 28, 2019

nreese commented Jan 29, 2019

thompsongl commented Jan 29, 2019

thompsongl commented Jan 30, 2019 • edited Loading

cchaos commented Jan 30, 2019

cchaos Jan 30, 2019

Choose a reason for hiding this comment

thompsongl commented Feb 1, 2019

thompsongl commented Feb 1, 2019

chandlerprall commented Feb 5, 2019

cchaos commented Feb 5, 2019

chandlerprall left a comment

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

thompsongl commented Feb 5, 2019

cchaos left a comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

thompsongl commented Jan 28, 2019 •

edited

Loading

thompsongl commented Jan 28, 2019 •

edited

Loading

cchaos commented Jan 28, 2019 •

edited

Loading

thompsongl commented Jan 30, 2019 •

edited

Loading