[Lens] Supports long legend values #107894

stratoula · 2021-08-09T13:41:55Z

Summary

Part of #41418

Adds two additional settings on the legend popover.
The switch is by default on, and the max lines number input is enabled and set to 1.
If the user turns off the switch, the input will be disabled and the legend items will display their whole context.
Max truncated lines are set to 5.
It is enabled for all visualization types

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

elasticmachine · 2021-08-10T10:00:41Z

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

dej611 · 2021-08-10T13:25:27Z

I am not so sure about the multiple options in the legend popover.

What if the max lines would be raised up to 3 (instead of 1) and just make truncation work without an explicit input for the user?
A tooltip on the label can explain that truncation will work if the label is longer and that should be enough I think.
Here I made a mockup with EUI with a range slider + input with max 10 lines for legends (10 lines is already a lot probably?): https://codesandbox.io/s/dazzling-fire-644y3

stratoula · 2021-08-10T13:32:30Z

So if I understand correctly, you don't want to cover the "zero" case which actually allows the users to display all the legend content. I am not sure about it tbh. I think is most powerful that way but of course, @MichaelMarcialis is the best to decide here. The explanation tooltip seems a good idea to me :)

dej611 · 2021-08-10T13:41:47Z

So if I understand correctly, you don't want to cover the "zero" case which actually allows the users to display all the legend content. I am not sure about it tbh. I think is most powerful that way but of course, @MichaelMarcialis is the best to decide here. The explanation tooltip seems a good idea to me :)

The upper bound of the range input can be raised if 10 lines are not enough to cover the whole content of the legend entry, but honestly I do not think too many cases will happen because of that.

stratoula · 2021-08-11T07:43:42Z

@elasticmachine merge upstream

stratoula · 2021-08-11T16:52:41Z

cc @AlonaNadler

AlonaNadler · 2021-08-11T19:04:10Z

LGTM thanks :)

MichaelMarcialis

This looks excellent, @stratoula! Well done. For your review, I've left two small text change comments.

Additionally, I have one question/request regarding the current handling of numbers greater than 5 in the maximum lines input. When typing a number greater than 5 and blurring the input, it currently triggers an error state. Instead, if a number greater than 5 is types, can we automatically change it to the highest supported line number (i.e. 5) on blur? That way it saves the user the trouble of correcting and also educates them what the highest value is.

Assuming you're able to address these, this PR looks good to me. Approving now so I don't hold you up.

x-pack/plugins/lens/public/shared_components/legend_settings_popover.tsx

MichaelMarcialis · 2021-08-12T01:00:39Z

So if I understand correctly, you don't want to cover the "zero" case which actually allows the users to display all the legend content. I am not sure about it tbh. I think is most powerful that way but of course, @MichaelMarcialis is the best to decide here. The explanation tooltip seems a good idea to me :)

Honestly, I'm a bit torn on this myself. I certainly appreciate the simplicity in what @dej611 is suggesting (i.e. truncation always on, with a single input to set the maximum lines). However, the version presented in this PR is more robust in that it supports a total disabling of truncation, which could potentially be helpful to users with odd edge cases (i.e. very lengthy labels).

Assuming it wouldn't be a huge pain to change it in the future, I'm thinking it may be best to keep it as it is here for now and bring it up for discussion with @ghudgins when he returns. Thoughts?

stratoula · 2021-08-12T06:09:45Z

I agree with this plan Michael. I also discussed it with Alona - as Graham is out - and she agrees to also give this option to the users. Let's go with it (as this is also the solution that we agreed with Graham) and I can discuss it again with him when he is back.

stratoula · 2021-08-12T06:20:05Z

@elasticmachine merge upstream

stratoula · 2021-08-12T07:00:58Z

This looks excellent, @stratoula! Well done. For your review, I've left two small text change comments.

Additionally, I have one question/request regarding the current handling of numbers greater than 5 in the maximum lines input. When typing a number greater than 5 and blurring the input, it currently triggers an error state. Instead, if a number greater than 5 is types, can we automatically change it to the highest supported line number (i.e. 5) on blur? That way it saves the user the trouble of correcting and also educates them what the highest value is.

Assuming you're able to address these, this PR looks good to me. Approving now so I don't hold you up.

Sure, I addressed that. When the user types a number greater than 5 and less than 1 (i.e. 0) I automatically change the value to the limits

stratoula · 2021-08-16T06:05:02Z

@elasticmachine merge upstream

mbondyra · 2021-08-17T08:14:11Z

@elasticmachine merge upstream

x-pack/plugins/lens/common/expressions/heatmap_chart/heatmap_legend.ts

x-pack/plugins/lens/common/expressions/xy_chart/legend_config.ts

x-pack/plugins/lens/public/heatmap_visualization/chart_component.tsx

x-pack/plugins/lens/public/shared_components/legend_settings_popover.tsx

mbondyra

Neither of my comments is blocking. I tested this feature on FF and works great!

kibanamachine · 2021-08-17T12:38:00Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.7MB	1.7MB	+6.1KB

Unknown metric groups

API count

id	before	after	diff
`lens`	216	220	+4

API count missing comments

id	before	after	diff
`lens`	200	202	+2

History

💛 Build #146047 was flaky 91edaff
💚 Build #145552 succeeded 5e32f14
💚 Build #144676 succeeded dd0fa94
💚 Build #144325 succeeded 0142cc3
💛 Build #143967 was flaky 18b7cdc

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

* [Lens] Supports multilines legend * Add a truncate legends switch * Add more unit tests * Add tooltip condition * Adress PR comments * Apply PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [Lens] Supports multilines legend * Add a truncate legends switch * Add more unit tests * Add tooltip condition * Adress PR comments * Apply PR comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

stratoula added 3 commits August 9, 2021 13:55

[Lens] Supports multilines legend

3b560fc

Add a truncate legends switch

0365fd4

Add more unit tests

7d1e0f5

stratoula changed the title ~~Lens long legend~~ [Lens] Supports long legend items Aug 10, 2021

stratoula changed the title ~~[Lens] Supports long legend items~~ [Lens] Supports long legend values Aug 10, 2021

Add tooltip condition

18b7cdc

stratoula added Feature:Lens release_note:enhancement v7.15.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Aug 10, 2021

stratoula marked this pull request as ready for review August 10, 2021 10:00

stratoula requested a review from a team August 10, 2021 10:00

stratoula requested a review from MichaelMarcialis August 10, 2021 10:29

Merge branch 'master' into lens-long-legend

0142cc3

MichaelMarcialis approved these changes Aug 12, 2021

View reviewed changes

x-pack/plugins/lens/public/shared_components/legend_settings_popover.tsx Outdated Show resolved Hide resolved

x-pack/plugins/lens/public/shared_components/legend_settings_popover.tsx Outdated Show resolved Hide resolved

kibanamachine and others added 2 commits August 12, 2021 02:20

Merge branch 'master' into lens-long-legend

adacf6f

Adress PR comments

dd0fa94

Merge branch 'master' into lens-long-legend

5e32f14

stratoula requested a review from mbondyra August 17, 2021 06:10

Merge branch 'master' into lens-long-legend

91edaff