Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[XY, Pie] Long legend values support #108365

Merged
merged 7 commits into from
Aug 19, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Aug 12, 2021

Summary

Part of #41418

  • Adds two additional settings on the options tab
  • 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.

Screenshot 2021-08-12 at 3 38 45 PM

Screenshot 2021-08-12 at 3 20 40 PM

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula added enhancement New value added to drive a business result Feature:Pie Chart Pie chart visualization feature Feature:XYAxis XY-Axis charts (bar, area, line) Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.15.0 v8.0.0 release_note:enhancement and removed enhancement New value added to drive a business result labels Aug 13, 2021
@stratoula stratoula marked this pull request as ready for review August 13, 2021 06:19
@stratoula stratoula requested a review from a team August 13, 2021 06:19
@elasticmachine
Copy link
Contributor

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

@dej611
Copy link
Contributor

dej611 commented Aug 13, 2021

Did a quick review check with Safari.

I found this strange bug where one legend item was not fully displayed:

Screenshot 2021-08-13 at 10 50 38

I thought that one first item label was actually been stored truncated, but then opening the Inspector Panel shows me the full version:

Screenshot 2021-08-13 at 10 50 45

@stratoula
Copy link
Contributor Author

@dej611 this happens because there is another setting in the pie chart, the Truncate text which is by default 100
image

I think I should group these settings together somehow. Let me think about it

@stratoula
Copy link
Contributor Author

stratoula commented Aug 13, 2021

Ok so first of all @dej611 has found a bug :)
I was using the truncate labels setting to also truncate the legends something that is wrong.
I fixed that in this PR and also I changed a bit the truncate labels setting to work only for labels that are positioned outside the chart and not inside. We had added this setting in the old implementation as all the labels were depicted outside the chart.

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

mbondyra commented Aug 18, 2021

Hey Stratoula, while testing, I noticed the behavior is different that in Lens - if you put more than 5 lines of legend (in the picture 1000, but works for 6 too), the setting is still respected instead of trimming to 5 and changing the setting. You can also save the vis in this state, but the setting will show the red line indicating it's invalid.
Is this a bug or we want it this way for some reason?
Screenshot 2021-08-18 at 15 40 53

As for the pie chart, I am not sure if this comes from this PR but there's also some problems with the legend positioning:
Screenshot 2021-08-18 at 15 45 50
Screenshot 2021-08-18 at 15 47 47

I'll keep investigating!

@mistic mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

stratoula commented Aug 19, 2021

@mbondyra I addressed the first two, thanx a lot!
About the last one, you have used filters and a large query, right? This is irrelevant with this PR.

On the legend actions popover we get the entire value and we display it as the popover title. Maybe we should truncate it. I will discuss it with Michael and Graham because it also happens in Lens but is still irrelevant with these changes :)
Btw, in the filters case, the user can add a custom label so this long title can be overwritten.

@mbondyra
Copy link
Contributor

I think that not truncating on filters is totally fine, but let's see what Graham and Michael say. Rechecking and approving :)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visDefaultEditor 126 127 +1

Async chunks

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

id before after diff
visTypePie 75.0KB 76.1KB +1.0KB
visTypeXy 114.0KB 114.8KB +812.0B
total +1.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visDefaultEditor 34.3KB 35.6KB +1.3KB
visTypePie 20.8KB 21.3KB +538.0B
visTypeXy 62.5KB 63.2KB +691.0B
total +2.5KB

History

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

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing the comments. Code LGTM. I tested it Chrome, works fine (the bits we talked about we can address later)

@stratoula stratoula added v7.15.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 19, 2021
@stratoula stratoula merged commit 4419efc into elastic:master Aug 19, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 19, 2021
* [XY, Pie] Long legend values support

* Update vislib snapshots

* Fix truncate labels to work only for slice labels positioned outside the chart

* Address PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 19, 2021
* [XY, Pie] Long legend values support

* Update vislib snapshots

* Fix truncate labels to work only for slice labels positioned outside the chart

* Address PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.15
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 19, 2021
* [XY, Pie] Long legend values support

* Update vislib snapshots

* Fix truncate labels to work only for slice labels positioned outside the chart

* Address PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
kibanamachine added a commit that referenced this pull request Aug 19, 2021
* [XY, Pie] Long legend values support

* Update vislib snapshots

* Fix truncate labels to work only for slice labels positioned outside the chart

* Address PR comments

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Pie Chart Pie chart visualization feature Feature:XYAxis XY-Axis charts (bar, area, line) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.15.0 v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants