[Vis: Default editor] EUIficate time interval control #34991

maryia-lapata · 2019-04-12T14:16:54Z

Summary

EUIfication of time interval control for aggregation parameter in Default Editor, Data tab.
Part of #30922

This PR contains:

Two interval inputs (interval selector and custom interval input ) were replaced with one EUIComboBox control which can handle single selection and custom selections.
Since now we have one control and one model that stores selected or custom interval, the customInterval parameter was removed. The migration script was added as well.
More help text was added under the control. Now help text with tooltip for scaled interval is displayed under the control.
validate-date-interval directive was removed since it isn't used anymore.

Screenshots:

Interval options

with Help text

with help text for scaled interval

with custom interval

with invalid custom interval

with empty value

with timeBase and help text from editorConfig

with timeBase and help text from editorConfig when interval is invalid

gif with selected options

gif with custom interval

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev-Docs

Removed `customInterval` from AggParam, changed `interval` value

We removed customInterval from AggParam. Now the custom interval is specified in the interval property. Also now predefined intervals such as Yearly, Daily, etc., is stored as a key in the interval property, e.g. interval: 'y', interval: 'd', interval: '2s'.

cchaos · 2019-04-15T18:25:23Z

@maryia-lapata Can you resolve the conflicts first?

cchaos · 2019-04-16T21:19:49Z

So I've been thinking about a way to improve these two inputs. We have the EuiComboBox which can handle single selection and custom selections. What if we could combine these two inputs into a single EuiComboBox?

It would look something like this:

We would need some help text to tell them how to type in a custom interval if they choose. And add validation text for if they've typed it in wrong.

I'd also like to move away from using the alert icon for messages that don't have to do with the validation of the input, but merely a message as to how the application is currently handling the selected option. Therefore, I'd highly suggest changing the label of the input from Interval to Minimum interval and when the application has to scale that interval do this:

maryia-lapata · 2019-04-17T12:12:25Z

@cchaos thank you for suggestions. I will try to implement them.

chrisdavies

LGTM. Just a review. I didn't pull it down.

elasticmachine · 2019-04-29T15:30:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

wylieconlon

Tested it out locally and it is working, but I have some comments about the style and naming. I'd like to review again before merging.

Also, how can I reproduce the "buckets are too large" case?

wylieconlon · 2019-04-29T21:52:17Z

src/legacy/ui/public/agg_types/controls/time_interval.tsx

+const tooLargeBucketsTooltip = (
+  <FormattedMessage
+    id="common.ui.aggTypes.timeInterval.createsTooLargeBucketsTooltip"
+    defaultMessage="This interval creates buckets that are too large to show in the selected time range, so it has been scaled up."


I wasn't able to reproduce this case- any suggestions?

Also, I believe the behavior should be to scale down, not up?

To reproduce this case you can use the following steps:

set Maximum bars to 1 in Advanced Settings,

create Area visualization

set Time range to Last 13 months

create Date Histogram aggregation and set Interval to 13M

Also, I believe the behavior should be to scale down, not up?

You're right. I updated the message.

wylieconlon · 2019-04-29T21:52:43Z

src/legacy/ui/public/agg_types/controls/time_interval.tsx

+const selectOptionHelpText = (
+  <FormattedMessage
+    id="common.ui.aggTypes.timeInterval.selectOptionHelpText"
+    defaultMessage="Select an option or create a custom value. Examples: 30s, 20m, 24h, 2d, 1w, 1M"


I would have appreciated text describing how to create a custom value. It was not clear to me that I had to type the custom value and hit enter.

When a user is typing in EUIComboBox, a tip appears:

src/legacy/ui/public/agg_types/utils.tsx

src/legacy/ui/public/agg_types/controls/time_interval.tsx

wylieconlon · 2019-04-29T22:16:05Z

src/legacy/ui/public/agg_types/controls/time_interval.tsx

+    ? []
+    : (aggParam.options || []).reduce(
+        (filtered: ComboBoxOption[], option: AggParamOption) => {
+          if (option.enabled ? option.enabled(agg) : true) {


if (option.enabled && option.enabled(agg))

Some options don't have enabled method and they should be displayed. The logic was moved from here https://github.com/elastic/kibana/pull/34991/files?file-filters%5B%5D=.js&hide-deleted-files=true#diff-a17d2b8a55911e4af0fdaacffbbe87afL85

elasticmachine · 2019-04-30T10:08:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-30T13:47:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

wylieconlon · 2019-04-30T18:56:29Z

Haven't re-run the code, but the changes since last review look good to me.

test/functional/services/combo_box.js

elasticmachine · 2019-05-01T20:19:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

dmlemeshko

Changes in functional tests reviewed. Tested on both Chrome/FF on MacOS - all good!

* EUIficate time interval control * Update tests * Remove empty option, update fix tests * Bind vis to scope for react component only * Combine two interval inputs into one EuiCombobox * Add error message * Add migration script; remove unused translations * Update fuctional tests * Update unit test * Update tests; refactoring * Use flow to invoke several functions * Update test * Refactoring * Reset options when timeBase * Add type for editorConfig prop * Add placeholder * Fix lint errors * Call write after interval changing * Fix code review comments * Make replace for model name global * Revert error catch * Remove old dependency * Add unit test for migration test * Fix message * Fix code review comments * Update functional test

EUIficate time interval control

b822888

This comment has been minimized.

Sign in to view

maryia-lapata added 3 commits April 15, 2019 09:54

Merge branch 'master' into vis-editor/time-interval

db04376

Update tests

9d20af2

Remove empty option, update fix tests

4f397f6

This comment has been minimized.

Sign in to view

Bind vis to scope for react component only

7911aa8

This comment has been minimized.

Sign in to view

maryia-lapata added Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes v7.2.0 v8.0.0 labels Apr 15, 2019

maryia-lapata marked this pull request as ready for review April 15, 2019 15:28

maryia-lapata requested review from timroes, cchaos and sulemanof April 15, 2019 15:28

timroes mentioned this pull request Apr 16, 2019

EUIficate vis editor data panel #30922

Closed

29 tasks

Merge branch 'master' into vis-editor/time-interval

fd642b0

This comment has been minimized.

Sign in to view

maryia-lapata added the WIP Work in progress label Apr 17, 2019

maryia-lapata changed the title ~~[Vis: Default editor] EUIficate time interval control~~ WIP [Vis: Default editor] EUIficate time interval control Apr 17, 2019

Merge branch 'master' into vis-editor/time-interval

32604d0

Combine two interval inputs into one EuiCombobox

735542e

This comment has been minimized.

Sign in to view

maryia-lapata added 2 commits April 22, 2019 09:52

Merge branch 'master' into vis-editor/time-interval

7b794c3

Add error message

e42d471

maryia-lapata added 2 commits April 29, 2019 14:59

Merge branch 'master' into vis-editor/time-interval

5571977

Remove old dependency

5f77403

chrisdavies approved these changes Apr 29, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Add unit test for migration test

17f7bde

maryia-lapata requested a review from dmlemeshko April 29, 2019 15:13

wylieconlon suggested changes Apr 29, 2019

View reviewed changes

maryia-lapata added 2 commits April 30, 2019 10:59

Merge branch 'master' into vis-editor/time-interval

f986a9c

Fix message

155af1d

Fix code review comments

5542184

wylieconlon approved these changes Apr 30, 2019

View reviewed changes

dmlemeshko requested changes May 1, 2019

View reviewed changes

test/functional/services/combo_box.js Show resolved Hide resolved

maryia-lapata added 2 commits May 1, 2019 21:57

Merge branch 'master' into vis-editor/time-interval

aaf9e05

Update functional test

f531484

maryia-lapata requested a review from dmlemeshko May 2, 2019 08:04

dmlemeshko approved these changes May 2, 2019

View reviewed changes

sulemanof merged commit 9c8894c into elastic:master May 3, 2019

sulemanof mentioned this pull request May 3, 2019

[7.x] [Vis: Default editor] EUIficate time interval control (#34991) #36006

Merged

sulemanof added the backported label May 3, 2019

maryia-lapata deleted the vis-editor/time-interval branch May 13, 2019 06:46

timroes mentioned this pull request Aug 1, 2019

Line visualization is messed up with Kibana 7.2.1 #42335

Closed

sulemanof mentioned this pull request Oct 18, 2019

Visualization interval scaled warning doesn't display correct interval #25065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vis: Default editor] EUIficate time interval control #34991

[Vis: Default editor] EUIficate time interval control #34991

maryia-lapata commented Apr 12, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cchaos commented Apr 15, 2019

This comment has been minimized.

cchaos commented Apr 16, 2019

maryia-lapata commented Apr 17, 2019

This comment has been minimized.

chrisdavies left a comment

This comment has been minimized.

elasticmachine commented Apr 29, 2019

wylieconlon left a comment

wylieconlon Apr 29, 2019

maryia-lapata Apr 30, 2019

wylieconlon Apr 29, 2019

maryia-lapata Apr 30, 2019

wylieconlon Apr 29, 2019

maryia-lapata Apr 30, 2019

elasticmachine commented Apr 30, 2019

elasticmachine commented Apr 30, 2019

wylieconlon commented Apr 30, 2019

elasticmachine commented May 1, 2019

dmlemeshko left a comment

[Vis: Default editor] EUIficate time interval control #34991

[Vis: Default editor] EUIficate time interval control #34991

Conversation

maryia-lapata commented Apr 12, 2019 • edited Loading

Summary

Checklist

For maintainers

Dev-Docs

Removed customInterval from AggParam, changed interval value

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cchaos commented Apr 15, 2019

This comment has been minimized.

cchaos commented Apr 16, 2019

maryia-lapata commented Apr 17, 2019

This comment has been minimized.

chrisdavies left a comment

Choose a reason for hiding this comment

This comment has been minimized.

elasticmachine commented Apr 29, 2019

💚 Build Succeeded

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Apr 29, 2019

Choose a reason for hiding this comment

maryia-lapata Apr 30, 2019

Choose a reason for hiding this comment

wylieconlon Apr 29, 2019

Choose a reason for hiding this comment

maryia-lapata Apr 30, 2019

Choose a reason for hiding this comment

wylieconlon Apr 29, 2019

Choose a reason for hiding this comment

maryia-lapata Apr 30, 2019

Choose a reason for hiding this comment

elasticmachine commented Apr 30, 2019

💚 Build Succeeded

elasticmachine commented Apr 30, 2019

💚 Build Succeeded

wylieconlon commented Apr 30, 2019

elasticmachine commented May 1, 2019

💚 Build Succeeded

dmlemeshko left a comment

Choose a reason for hiding this comment

maryia-lapata commented Apr 12, 2019 •

edited

Loading

Removed `customInterval` from AggParam, changed `interval` value