[Input controls vis] replace react-input-range with EuiDualRange #31119

nreese · 2019-02-14T14:07:43Z

Replace react-input-range with EuiDualRange for Input control range slider

PR creates ValidatedDualRange which will also be used in the Maps application

elasticmachine · 2019-02-14T14:07:45Z

Pinging @elastic/kibana-app

elasticmachine · 2019-02-14T17:42:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos · 2019-02-14T19:23:43Z

@nreese Is this ready for review? It doesn't seem to be rendering correctly:

…s_dual_range

elasticmachine · 2019-02-14T21:59:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-14T22:00:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2019-02-14T22:14:04Z

Is this ready for review? It doesn't seem to be rendering correctly

Yes, the PR is ready for review. The flight dashboard provided a lot of decimal places which broke the formatting. I have fixed those problems now.

elasticmachine · 2019-02-14T23:19:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos · 2019-02-15T16:03:48Z

Unfortunately, it looks like we have more work to do on the the EUI dual slider

cc @thompsongl

nreese · 2019-02-15T16:45:36Z

Unfortunately, it looks like we have more work to do on the the EUI dual slider

@cchaos Are you going to open an EUI issue or do you want me to open an issue?

cchaos · 2019-02-15T20:36:32Z

I can make an issue

cchaos

Also, thank you for fixing the validation error that expected super precise values to be selected.

cchaos · 2019-02-15T21:33:37Z

src/legacy/core_plugins/input_control_vis/public/components/vis/range_control.js

+        showInput
+        showRange
+        showTicks
+        ticks={ticks}


So the main issue that was happening was with the ticks. I see that you're only adding the min and max as ticks, which isn't necessary since we have the showLabels prop which automatically appends the min and max values passed. Like so:

However, I was going to suggest auto-filling the inputs with the min/max numbers so the range wasn't in a weird state like the above, but basically looked like it has the whole range selected. Therefore, if we do that, then we don't need the showLabels because the inputs will already show those numbers and it's quite easy to reset to the whole range with the reset button:

I've do have to put in a fix for the input sizing and I saw a few other bugs, but this should allow you to continue with the PR.

I see that you're only adding the min and max as ticks, which isn't necessary since we have the showLabels prop which automatically appends the min and max values passed

The problem with showLabels is that it displays the labels inline with the slider. For input controls, horizontal space is at a premium so I wanted the labels below the slider to save horizontal space. I also really like the ability to select a tick to set the value

However, I was going to suggest auto-filling the inputs with the min/max numbers so the range wasn't in a weird state like the above

You can not pre set the values to the entire range because then that suggests that there is a range filter already applied. The default state is that the slider has no values. Then you can use the slider to specify a range.

Then we need to somehow disconnect empty values from the look of the range because the range looks broken.

Also the ticks are broken right now because custom ticks don't correctly take into account the min value, it's always based on 0 being the min. I'm looking into it now.

cchaos · 2019-02-15T21:35:31Z

src/legacy/core_plugins/input_control_vis/public/components/vis/range_control.js

+  static getDerivedStateFromProps(nextProps, prevState) {
+    const nextValue = nextProps.control.hasValue()
+      ? [nextProps.control.value.min, nextProps.control.value.max]
+      : ['', ''];


I wouldn't pass blank values if the user hasn't changed them, I would set them to the min/max values available.

The empty state is not value. Setting to min and max makes the user assume there is a filter applied which is not the case

Doesn't the absence of values mean that the full range is in play, though?

Doesn't the absence of values mean that the full range is in play, though?

No, the absence of values means that there is nothing set. Logically that does mean that the entire range is in play but setting a range and having the range be unset are 2 important UI states.

When the range is unset, there is not range filtering the results. The UI needs to be empty to show the user that no filtering is being applied.

When a range is set, there is a range filtering the results. The UI needs to show the user the range so they can visually see how their data is being filtered.

The min/max range will change depending on user selected time, active filters, and query. Showing the entire range as selected is misleading when no range filter is getting applied.

cchaos · 2019-02-15T21:36:10Z

src/legacy/ui/public/validated_range/is_range_valid.js

+    isValid = false;
+    errorMessage = formatMessage({
+      id: 'common.ui.dualRangeControl.mustSetBothErrorMessage',
+      defaultMessage: 'both lower and upper value must be set'


Don't forget to always use sentence case

Suggested change

defaultMessage: 'both lower and upper value must be set'

defaultMessage: 'Both lower and upper values must be set'

cchaos · 2019-02-15T21:36:25Z

src/legacy/ui/public/validated_range/is_range_valid.js

+    isValid = false;
+    errorMessage = formatMessage({
+      id: 'common.ui.dualRangeControl.outsideOfRangeErrorMessage',
+      defaultMessage: 'must be between {min} and {max}'


Suggested change

defaultMessage: 'must be between {min} and {max}'

defaultMessage: 'Values must be on or between {min} and {max}'

cchaos · 2019-02-15T21:36:54Z

src/legacy/ui/public/validated_range/is_range_valid.js

+    isValid = false;
+    errorMessage = formatMessage({
+      id: 'common.ui.dualRangeControl.upperValidErrorMessage',
+      defaultMessage: 'upper must be greater or equal to lower'


Suggested change

defaultMessage: 'upper must be greater or equal to lower'

defaultMessage: 'Upper value must be greater or equal to lower value'

lukeelmers

Overall LGTM! Added a few minor notes.

Only thing I'm seeing that feels amiss is a prop type warning on one of the sample data dashboards that includes the range slider:

Warning: Failed prop type: The prop `value` is marked as required in `EuiRangeInput`, but its value is `null`.

Appears on initial page load when viewing the dashboard that comes with the web logs sample data, but I don't see it when creating a slider from within the visualize app.

src/legacy/ui/public/validated_range/is_range_valid.test.js

thompsongl · 2019-02-19T16:23:54Z

Unfortunately, it looks like we have more work to do on the the EUI dual slider

cc @thompsongl

The custom tick min value looks to be a bug, but I'm not exactly sure what to do about not passing values. In the 1-4 range case, aren't min: 1 & max: 4 still used as filters for the resulting data? If not, how is that range determined?

cchaos · 2019-02-19T16:36:01Z

@thompsongl I've got a branch going that will fix that bug (it just doesn't take the min value into consideration).

thompsongl · 2019-02-19T18:05:01Z

@cchaos If it's valid for a range to be set but to also not participate in that range (empty string values), do we need to have some other visual state?
As a user, I see that the values must be between 1 and 4, but also that it's acceptable for the values to not be between 1 and 4.
(We can certainly move this to a EUI issue if that makes more sense)

cchaos · 2019-02-26T17:53:02Z

@nreese The EuiRange/EuiDualRange fixes went out with EUI 7.3.0. You should be able to update and get those fixes now.

nreese · 2019-02-26T17:58:31Z

The EuiRange/EuiDualRange fixes went out with EUI 7.3.0. You should be able to update and get those fixes now.

waiting on #32009 to update Kibana's EUI version

nreese · 2019-03-07T14:15:58Z

@lukeelmers @cchaos This PR is ready for a final review.

elasticmachine · 2019-03-07T14:30:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-07T15:01:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-07T15:45:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…s any text, update snapshots

elasticmachine · 2019-03-07T16:50:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-07T17:54:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

Code all LGTM, no concerns there.

In re-testing locally, there are just two things I'm still noticing... specifically looking at the Web Traffic dashboard that comes with the sample data:

Might be more of an EUI question, but sliding the input control is still somewhat choppy -- wondering if maybe we need to debounce the onChange even more? Hard to tell if it is a debounce issue, or some other issue with the click events to actually drag the slider, but you can't just click and drag smoothly anymore like you could in the old implementation.
Still getting a propType warning. This occurs when I apply a filter via the range slider, then click in each of the text boxes and hit delete/backspace to reset the slider. Interestingly, I can't reproduce this one consistently. Like 25% of the time.

Warning: Failed prop type: The prop `value` is marked as required in `EuiRangeInput`, but its value is `null

Here's a screencap showing both behaviors:

thompsongl · 2019-03-11T15:34:45Z

Still getting a propType warning.

Appears that null sometimes gets passed as one of the initial value array values.
The validation at https://github.com/elastic/kibana/pull/31119/files#diff-a2c37d103458f78d5263f7a848c39924R50 (control.hasValue()) guards against both values being null, but not min or max individually

nreese · 2019-03-11T21:25:19Z

(control.hasValue()) guards against both values being null, but not min or max individually

Thanks for pointing this out. I have corrected the problem and now min or max of null will set the value to ''.

lukeelmers

Tested again and the choppiness I was experiencing before seems to be gone. Unclear if it had something to do with the recent updates, or if my browser was just laggy at the time of testing.

Either way, LGTM! I'm consistently getting smooth dragging now.

elasticmachine · 2019-03-11T22:15:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

[Input controls vis] replace react-input-range with EuiDualRange

ab01b40

nreese added Feature:Input Control Input controls visualization Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.2.0 labels Feb 14, 2019

nreese requested a review from a team as a code owner February 14, 2019 14:07

nreese added the chore label Feb 14, 2019

remove unneeded styles

42e3bcf

nreese requested a review from lukeelmers February 14, 2019 14:36

correctly format I18n with variables

94dc5a5

Merge branch 'master' of github.com:elastic/kibana into input_control…

2a82c6d

…s_dual_range

fix problems with long decimals

c17879d

fix one test case

69fde71

cchaos suggested changes Feb 15, 2019

View reviewed changes

lukeelmers reviewed Feb 15, 2019

View reviewed changes

thomasneirynck mentioned this pull request Mar 1, 2019

[Maps] Use dual range component #32273

Merged

nreese added 5 commits March 7, 2019 06:37

merge with master

cf036d4

clean-up error message text, remove extra commas in jest test

74320ad

fix bug where values were returned as strings

0cc4468

use i18n.translate instead of passing formatMessage into isRangeValid

392ce99

assert error message in isRangeValid unit test

fbf2be7

changes from node scripts/i18n_check --fix

81b6d27

remove localization from RangeControl component since it no longer ha…

a2587c8

…s any text, update snapshots

another snapshot update

c0ccc3a

lukeelmers reviewed Mar 9, 2019

View reviewed changes

nreese added 2 commits March 11, 2019 15:18

merge with master

9cc4f94

ensure min and max are not null

4721470

check for value before checking if min/max is null

38d64de

lukeelmers approved these changes Mar 11, 2019

View reviewed changes

nreese merged commit b5c958e into elastic:master Mar 11, 2019

nreese mentioned this pull request Mar 11, 2019

[7.x] [Input controls vis] replace react-input-range with EuiDualRange (#31119) #32940

Merged

nreese added the release_note:skip Skip the PR/issue when compiling release notes label May 22, 2019

nreese mentioned this pull request Nov 9, 2022

[Controls] Customize Decimal Places & Step Size On Range & Time Sliders #140329

Closed

	defaultMessage: 'both lower and upper value must be set'
	defaultMessage: 'Both lower and upper values must be set'

	defaultMessage: 'must be between {min} and {max}'
	defaultMessage: 'Values must be on or between {min} and {max}'

	defaultMessage: 'upper must be greater or equal to lower'
	defaultMessage: 'Upper value must be greater or equal to lower value'

[Input controls vis] replace react-input-range with EuiDualRange #31119

[Input controls vis] replace react-input-range with EuiDualRange #31119

Conversation

nreese commented Feb 14, 2019 • edited Loading

elasticmachine commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

💔 Build Failed

cchaos commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

💔 Build Failed

elasticmachine commented Feb 14, 2019

💔 Build Failed

nreese commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

💔 Build Failed

cchaos commented Feb 15, 2019

nreese commented Feb 15, 2019

cchaos commented Feb 15, 2019

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese Feb 15, 2019 • edited Loading

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

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

thompsongl commented Feb 19, 2019 • edited Loading

cchaos commented Feb 19, 2019

thompsongl commented Feb 19, 2019

cchaos commented Feb 26, 2019

nreese commented Feb 26, 2019

nreese commented Mar 7, 2019

elasticmachine commented Mar 7, 2019

💔 Build Failed

elasticmachine commented Mar 7, 2019

💔 Build Failed

elasticmachine commented Mar 7, 2019

💔 Build Failed

elasticmachine commented Mar 7, 2019

💔 Build Failed

elasticmachine commented Mar 7, 2019

💚 Build Succeeded

lukeelmers left a comment • edited Loading

Choose a reason for hiding this comment

thompsongl commented Mar 11, 2019

nreese commented Mar 11, 2019

lukeelmers left a comment

Choose a reason for hiding this comment

elasticmachine commented Mar 11, 2019

💚 Build Succeeded

nreese commented Feb 14, 2019 •

edited

Loading

nreese Feb 15, 2019 •

edited

Loading

thompsongl commented Feb 19, 2019 •

edited

Loading

lukeelmers left a comment •

edited

Loading