[Lens] Display legend inside chart #105571

stratoula · 2021-07-14T11:03:05Z

Summary

This PR adds more options to the legend popover of the XY axis charts to enable the users to display the legend inside the chart.

Options that are added:

Location button group. Users can select if they want their legend to appear inside or outside (existing functionality) the chart
In the case of inside there are additional settings:
- Configure the vertical and horizontal alignments
- Configure the number of floating columns

All settings are disabled if the hide display option is selected.

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-07-15T10:53:46Z

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

stratoula · 2021-07-15T10:54:10Z

@elasticmachine merge upstream

dej611

I'm not convinced by the Alignment field:

would it be better to have a group of 4 buttons pointing to each screen corner? (quick mockup):

Or even something more minimal with just the arrow pointing to the corner.

Is the Show value feature broken? I've tried to enable it but it does not work.

I find the Number of columns option very useful: it would be nice to extend it also to the external legend positioning (maybe followup)

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

ghudgins · 2021-07-15T14:56:59Z

@MichaelMarcialis what do you think of @dej611's question here #105571 (review)

wylieconlon

I agree with @dej611 that the current horizontal/vertical controls are using confusing icons. EUI provides 4 icons like editorPositionTopLeft which we should probably use to control both horizontal and vertical position at the same time.

@dej611 I tested the "show value" option and it works correctly.

Is it an elastic-charts feature that there is a background color assigned on hover? Is this something that works in dark mode?

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

dej611 · 2021-07-16T12:52:13Z

Is it an elastic-charts feature that there is a background color assigned on hover? Is this something that works in dark mode?

I've forgotten to report one point about that: there's some blending issue with the background color and the chart color.

MichaelMarcialis · 2021-07-16T13:49:12Z

@MichaelMarcialis what do you think of @dej611's question here #105571 (review)

I agree with @dej611's suggested change. As @wylieconlon already noted, we can use the following EUI icons to represent each corner in a single EuiButtonGroup:

editorPositionBottomLeft
editorPositionBottomRight
editorPositionTopLeft
editorPositionTopRight

MichaelMarcialis

This is looking good, @stratoula! I've left a few comments for your review:

Rather than using two separate/distinct form rows and labels for "Position" and "Alignment," could we just have a single form row called "Alignment," and change the contained button group depending on which location modes (inside/outside) is selected? I think doing so will be less confusing to users, as the two perform very similar functions, despite having different button group options.
Currently, the "Number of columns" form row disappears when the "Location" is set to "Outside." However, other form inputs within this menu become disabled rather than hidden when they are unavailable (but can still be re-enabled in the current context). As such, can we instead disable the "Number of columns" form input when "Location" is set to "Outside," rather than hiding it altogether?
For situations when a form input is disabled due to currently selected options, can we indicate to users how they can go about re-enabling that form input? For example, when "Display" is set to "Hide," could we have tooltips that appear on hover of the "Location," "Alignment," "Number of columns," and "Show value" inputs that states Requires legend to be shown? Similarly, when "Location" is set to "Outside", the now disabled "Number of columns" input can have a tooltip on hover that states Requires legend to be located inside visualization.

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

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

stratoula · 2021-07-19T06:21:06Z

@elasticmachine merge upstream

stratoula · 2021-07-19T09:42:43Z

Is it an elastic-charts feature that there is a background color assigned on hover? Is this something that works in dark mode?

Yes @wylieconlon it is.
Here how it works in dark mode

stratoula · 2021-07-19T12:23:19Z

Thank you all for the alignment buttons suggestion. It is much better that way!

@MichaelMarcialis I have addressed all your comments (I hope).

Changed the alignment icon buttons to the suggested ones
Position is renamed and used Alignement everywhere
Added tooltips to inform the users on how to enable these settings
Changed the position icons order to what you suggested
Changed the slider to number input

Can you take another look? Thanx :)

wylieconlon

One more comment, maybe for @markov00: it seems like the inner legend + columns option leads to overflow pretty often, especially when the # of columns is higher than the available width. Is this something that the charts library can detect and solve automatically?

markov00 · 2021-07-20T08:31:13Z

One more comment, maybe for @markov00: it seems like the inner legend + columns option leads to overflow pretty often, especially when the # of columns is higher than the available width. Is this something that the charts library can detect and solve automatically?

Something like that can be done probably, but my suggestion is to reduce the max number of columns to 4 max 5 for now.

stratoula · 2021-07-20T09:00:30Z

Something like that can be done probably, but my suggestion is to reduce the max number of columns to 4 max 5 for now.

I agree, I reduced it to max5 for now

MichaelMarcialis

Thanks for making those requested changes, @stratoula. The addition of the tooltips did create one small bug, where when the tooltips are present the elements contained within collapse down (due to the tooltip container being inline by default). You should be able to fix this by applying a display="block" prop to the related EuiToolTip components.

Otherwise, this looks good to me once the above is addressed. Approving now so I don't hold you up.

dej611 · 2021-07-20T14:21:41Z

Something like that can be done probably, but my suggestion is to reduce the max number of columns to 4 max 5 for now.

I agree, I reduced it to max5 for now

Just tested and the 5 max limit imposed is only checked if the user uses the input number steppers handles. Probably a validation should be set on top of it for user's manual input?

kibanamachine · 2021-07-20T16:55:11Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	703	704	+1

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.5MB	1.5MB	+10.9KB

Unknown metric groups

API count

id	before	after	diff
`lens`	181	185	+4

History

💚 Build #139653 succeeded 4b2819c
💚 Build #139340 succeeded 0693aff
💚 Build #139255 succeeded 51f8fcc
💚 Build #138719 succeeded ddc40da
💚 Build #138666 succeeded e01f85d

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

* [Lens] Legend inside chart * Apply design feedback * Add unit tests * Update snapshot * Add disabled state and unit tests * revert css change * Address PR comments * Reduce the max columns to 5 * Address last comments * minor * Add a comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* [Lens] Legend inside chart * Apply design feedback * Add unit tests * Update snapshot * Add disabled state and unit tests * revert css change * Address PR comments * Reduce the max columns to 5 * Address last comments * minor * Add a comment Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…y-show-migrate-to-authzd-users * 'master' of github.com:elastic/kibana: (48 commits) [Canvas] Expression shape (elastic#103219) [FTR] Skips Vega tests [Sample data] Use Lens in ecommerce data (elastic#106039) [APM] Backends inventory & overview page routes (elastic#106223) [TSVB] Add more functional tests for Gauge and TopN (elastic#105361) Add toggle to enable/disable rule install from SOs (elastic#106189) Improve unit test coverage of FS API calls (elastic#106242) Remove recursive plugin status in meta field (elastic#106286) [Ingest pipelines] add community id processor (elastic#103863) [XY axis] Fixes the values inside bar charts (elastic#106198) [data.search] Set default expiration to 1m if search sessions are disabled (elastic#105329) set the doc title when navigating to reporting and unset when navigating away (elastic#106253) [Lens] Display legend inside chart (elastic#105571) [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid` (elastic#106199) [Security Solutions] Removes the elastic legacy client from lists and security_solution plugins (elastic#106130) [Enterprise Search] Require security plugin in 8.0 (elastic#106307) [DOCS] Updates screenshots in Dev Tools docs (elastic#105859) [DOCS] Updates text and screenshots in tags doc (elastic#105853) [Alerting] Allow rule types to extract/inject saved object references on rule CRU (elastic#101896) Jest and Storybook fixes (elastic#104991) ... # Conflicts: # x-pack/plugins/reporting/public/plugin.ts

stratoula added 5 commits July 13, 2021 15:22

[Lens] Legend inside chart

3c8ab98

Apply design feedback

075514b

Add unit tests

fd994e6

Update snapshot

6e43e68

Add disabled state and unit tests

c3c5327

stratoula changed the title ~~Lens legend in chart~~ [Lens] Display legend inside chart Jul 15, 2021

revert css change

e01f85d

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

stratoula marked this pull request as ready for review July 15, 2021 10:53

stratoula requested a review from a team July 15, 2021 10:53

stratoula requested a review from MichaelMarcialis July 15, 2021 10:53

Merge branch 'master' into lens-legend-in-chart

ddc40da

dej611 reviewed Jul 15, 2021

View reviewed changes

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

wylieconlon approved these changes Jul 15, 2021

View reviewed changes

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

MichaelMarcialis reviewed Jul 16, 2021

View reviewed changes

Merge branch 'master' into lens-legend-in-chart

51f8fcc

Address PR comments

0693aff

wylieconlon reviewed Jul 19, 2021

View reviewed changes

Merge with master and resolve conflicts

a458320

Reduce the max columns to 5

4b2819c

dej611 approved these changes Jul 20, 2021

View reviewed changes

MichaelMarcialis approved these changes Jul 20, 2021

View reviewed changes

stratoula added 3 commits July 20, 2021 17:43

Address last comments

43bec2e

minor

4c8a25d

Add a comment

5933ab7

stratoula merged commit 1bfdc72 into elastic:master Jul 21, 2021

stratoula mentioned this pull request Jul 21, 2021

[7.x] [Lens] Display legend inside chart (#105571) #106351

Merged

dej611 mentioned this pull request Jul 21, 2021

[Lens] New legend inside chart can be more smart in terms of layout #106372

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Display legend inside chart #105571

[Lens] Display legend inside chart #105571

stratoula commented Jul 14, 2021 •

edited

Loading

elasticmachine commented Jul 15, 2021

stratoula commented Jul 15, 2021

dej611 left a comment

ghudgins commented Jul 15, 2021

wylieconlon left a comment

dej611 commented Jul 16, 2021

MichaelMarcialis commented Jul 16, 2021

MichaelMarcialis left a comment

stratoula commented Jul 19, 2021

stratoula commented Jul 19, 2021

stratoula commented Jul 19, 2021

wylieconlon left a comment

markov00 commented Jul 20, 2021

stratoula commented Jul 20, 2021 •

edited

Loading

MichaelMarcialis left a comment

dej611 commented Jul 20, 2021

kibanamachine commented Jul 20, 2021

API count

[Lens] Display legend inside chart #105571

[Lens] Display legend inside chart #105571

Conversation

stratoula commented Jul 14, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Jul 15, 2021

stratoula commented Jul 15, 2021

dej611 left a comment

Choose a reason for hiding this comment

ghudgins commented Jul 15, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

dej611 commented Jul 16, 2021

MichaelMarcialis commented Jul 16, 2021

MichaelMarcialis left a comment

Choose a reason for hiding this comment

stratoula commented Jul 19, 2021

stratoula commented Jul 19, 2021

stratoula commented Jul 19, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

markov00 commented Jul 20, 2021

stratoula commented Jul 20, 2021 • edited Loading

MichaelMarcialis left a comment

Choose a reason for hiding this comment

dej611 commented Jul 20, 2021

kibanamachine commented Jul 20, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

API count

History

stratoula commented Jul 14, 2021 •

edited

Loading

stratoula commented Jul 20, 2021 •

edited

Loading