[Lens] Don't allow values outside of range for number of top values #78734

mbondyra · 2020-09-29T08:26:30Z

Summary

Fixes #61780
It limits the user to only type values in the range. I also changed the range from [1,20] to [1,100] as it was one of the suggestions. It's open to discussion though.

…es_out_of_range

elasticmachine · 2020-09-29T08:26:33Z

Pinging @elastic/kibana-app (Team:KibanaApp)

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms.test.tsx

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms.tsx

change range to 1-100

dej611

Tested the PR in Safari, Firefox and Chrome.
It is ok 👍 .

A minor improvement, to me, would be if the number input would also handle the empty string as transition value (ie 1 => 25) rather than fallback to 1.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms.tsx

…_values_out_of_range

dej611

I think that the behaviour is somewhat logically correct here, but the fact that the chart does not render the MAX_NUMBER_OF_VALUES here instead of my out of bound 300 does not convince me 100%. What do you think?

I would expect in this case to not render 300 bars, rather 100 at least as the range input moves to that. But I'm open to alternative solutions.

...gins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx

mbondyra · 2020-09-29T18:20:26Z

I can see both bad and good sides of changing the values to the border ones in case of outside of range (that's probably expected for some users) and not doing it (user that accidentally types something won't modify their data by mistake) – I don't have a strong opinion so I'll stick to yours :) Updated!

wylieconlon

Tested the behavior and it mostly LGTM, just found one edge case.

wylieconlon · 2020-09-29T19:12:36Z

...gins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx

+
+type PropType<C> = C extends React.ComponentType<infer P> ? P : unknown;
+// Add ticks to EuiRange component props
+const FixedEuiRange = (EuiRange as unknown) as React.ComponentType<


Can you submit a PR to Eui to fix this?

Now looking at this code, I think it should already be fixed so I'll just remove it.

wylieconlon · 2020-09-29T19:13:44Z

...gins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx

+  onChange: (value: number) => void;
+}) => {
+  const MIN_NUMBER_OF_VALUES = 1;
+  const MAX_NUMBER_OF_VALUES = 100;


I'm okay with setting it 100 based on our "best practices" case, but we could also set it based on the MAX_BUCKETS ui setting. What do you think?

I like the idea of having it in the ui settings, but right now the set value for metrics:max_buckets is 2000. I checked in Visualize that we indeed allow terms of that many values, but the rendering is so slow (in my computer around a minute, if it doesn't stall completely) that it's completely unusable. And as it's easier to just use EuiRange to accidentally slide the value up than manually type it as we do in Visualize, I'd rather stick to maximum of 100 for lens. We could also add it as a separate setting in advanced settings if you think it's worth it.

I would go forward with the PR in its current state as it's an improvement over the behavior in the old version. Long term we can rethink this UI (maybe a slider without text input and a free input as an "advanced mode"?) - but IMHO that's out of scope for this PR.

wylieconlon · 2020-09-29T19:14:36Z

...gins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx

+      aria-label={i18n.translate('xpack.lens.indexPattern.terms.size', {
+        defaultMessage: 'Number of values',
+      })}
+    />


I found it confusing that I could be in an invalid state as long as I had focus, and the red highlighting would only appear once I blur the input:

Yeah, I noticed that too, but this is the internal behaviour of EuiRange - even adding isInvalid prop with some conditions doesn't help - the condition 'is fired' on blur and not on change, so it won't help. I'll speak to eui team to see if they can do something about. For now, do you think it's a blocker to merge this one?

Opening an issue for EUI is worth it, but it shouldn't block the PR IMHO

flash1293 · 2020-09-30T12:55:32Z

@elasticmachine merge upstream

flash1293

Tested in Chrome and Firefox and LGTM - the behavior seems predictable and understandable to me. Showing the invalid state on the input right away would be a nice touch but we can fix that later IMHO.

mbondyra · 2020-10-01T08:21:50Z

@elasticmachine merge upstream

kibanamachine · 2020-10-01T09:59:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 94930b6

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`lens`	560	561	+1

async chunks size

id	before	after	diff
`lens`	995.6KB	997.8KB	+2.3KB

History

💔 Build #78268 failed 74fb2b9
💚 Build #78178 succeeded 572ffa7
💔 Build #78017 failed 3da0ad1
💔 Build #77964 failed a768f96
💔 Build #77956 failed 40dc7ce

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…lastic#78734)

…78734) (#79094)

mbondyra added 2 commits September 28, 2020 18:26

[Lens] Don't allow outside of range size of terms

fafff36

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

4a2cff2

…es_out_of_range

mbondyra added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.10.0 labels Sep 29, 2020

mbondyra requested a review from a team September 29, 2020 08:26

mbondyra commented Sep 29, 2020

View reviewed changes

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms.test.tsx Outdated Show resolved Hide resolved

mbondyra commented Sep 29, 2020

View reviewed changes

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms.test.tsx Outdated Show resolved Hide resolved

mbondyra commented Sep 29, 2020

View reviewed changes

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms.tsx Outdated Show resolved Hide resolved

Apply suggestions from code review

5993d11

change range to 1-100

mbondyra requested review from wylieconlon, flash1293 and dej611 September 29, 2020 13:21

dej611 reviewed Sep 29, 2020

View reviewed changes

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms.tsx Outdated Show resolved Hide resolved

mbondyra added 3 commits September 29, 2020 15:54

Merge commit '6f3301232cae6e2c238870233718fbd3f6f03c24' into lens/top…

8598569

…_values_out_of_range

fix: allow string, performance problems, tests

40dc7ce

types

a768f96

mbondyra force-pushed the lens/top_values_out_of_range branch from 3c5563a to a768f96 Compare September 29, 2020 16:35

dej611 reviewed Sep 29, 2020

View reviewed changes

...gins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx Outdated Show resolved Hide resolved

address CR

3da0ad1

wylieconlon reviewed Sep 29, 2020

View reviewed changes

fix tests

572ffa7

Merge branch 'master' into lens/top_values_out_of_range

74fb2b9

flash1293 approved these changes Sep 30, 2020

View reviewed changes

Merge branch 'master' into lens/top_values_out_of_range

94930b6

mbondyra merged commit d11da32 into elastic:master Oct 1, 2020

mbondyra added a commit to mbondyra/kibana that referenced this pull request Oct 1, 2020

[Lens] Don't allow values outside of range for number of top values (e…

f783bc2

…lastic#78734)

mbondyra mentioned this pull request Oct 1, 2020

[7.x] [Lens] Don't allow values outside of range for number of top values (#78734) #79094

Merged

mbondyra deleted the lens/top_values_out_of_range branch October 1, 2020 11:58

mbondyra added a commit that referenced this pull request Oct 1, 2020

[Lens] Don't allow values outside of range for number of top values (#…

827b4a5

…78734) (#79094)

wylieconlon mentioned this pull request Mar 10, 2021

[Lens] Restructure top values dimension editor #93912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Don't allow values outside of range for number of top values #78734

[Lens] Don't allow values outside of range for number of top values #78734

mbondyra commented Sep 29, 2020

elasticmachine commented Sep 29, 2020

dej611 left a comment

dej611 left a comment •

edited

Loading

mbondyra commented Sep 29, 2020

wylieconlon left a comment

wylieconlon Sep 29, 2020

mbondyra Sep 30, 2020

wylieconlon Sep 29, 2020

mbondyra Sep 30, 2020

flash1293 Sep 30, 2020

wylieconlon Sep 29, 2020

mbondyra Sep 30, 2020

flash1293 Sep 30, 2020

flash1293 commented Sep 30, 2020

flash1293 left a comment

mbondyra commented Oct 1, 2020

kibanamachine commented Oct 1, 2020

[Lens] Don't allow values outside of range for number of top values #78734

[Lens] Don't allow values outside of range for number of top values #78734

Conversation

mbondyra commented Sep 29, 2020

Summary

elasticmachine commented Sep 29, 2020

dej611 left a comment

Choose a reason for hiding this comment

dej611 left a comment • edited Loading

Choose a reason for hiding this comment

mbondyra commented Sep 29, 2020

wylieconlon left a 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

Choose a reason for hiding this comment

flash1293 commented Sep 30, 2020

flash1293 left a comment

Choose a reason for hiding this comment

mbondyra commented Oct 1, 2020

kibanamachine commented Oct 1, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

History

dej611 left a comment •

edited

Loading