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

[Lens] Waffle visualization type #119339

Merged
merged 10 commits into from
Dec 2, 2021
Merged

[Lens] Waffle visualization type #119339

merged 10 commits into from
Dec 2, 2021

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Nov 22, 2021

Closes: #107059

Summary

Add waffle chart as a Visualization Type to the set of partition visualizations in Lens (alongside Pie, Treemap, Moasaic). It shows a parts to whole relationship, like pie charts or treemaps, but sometimes a better option than these. It is distinct from the gridded charts in APM that show specific units and were titled waffle map.

Screens

Chart:
image

Options:
image

Small values warning:
image

@alexwizp alexwizp force-pushed the 107059 branch 3 times, most recently from 212aef9 to 6111d8a Compare November 23, 2021 12:51
@alexwizp alexwizp changed the title [WIP][Lens] Waffle visualization type [Lens] Waffle visualization type Nov 23, 2021
@alexwizp alexwizp marked this pull request as ready for review November 24, 2021 11:19
@alexwizp alexwizp requested a review from a team as a code owner November 24, 2021 11:19
@alexwizp alexwizp self-assigned this Nov 24, 2021
@alexwizp alexwizp added v8.1.0 backport:skip This commit does not require backporting Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Nov 24, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested locally on Safari, and I have some questions (no necessarily to be addressed in this PR):

  1. Can we make the legend Show by default when in Auto? I see no other way to understand categories in this type of chart. Not even sure an Auto makes sense here, rather than a simple Show/Hide.

Screenshot 2021-11-24 at 17 09 18

  1. I assumed the sorting of each waffle square goes from "big" to "small" categories from top to bottom. I see this criteria applied here:

Screenshot 2021-11-24 at 17 12 06

but then changing categories I see random placement (easy to reproduce with array fields):

Screenshot 2021-11-24 at 17 11 10

  1. legend labels have some issue when they are number-like?

String categories make a rational use of the space for the legend:

Screenshot 2021-11-24 at 17 12 06

Number-like don't:

Screenshot 2021-11-24 at 17 11 48

  1. Legend entries have a different order than the chart categories:

Screenshot 2021-11-24 at 17 13 38

  1. When transitioning from another chart to Waffle, sometimes with a partial configuration the "too small data" warning is shown, but the chart no:

xy_to_waffle_warning

  1. I have some conflictual feelings for this Labels popover menu with only percent precision in it. I see it may be useful, but:
  • the nature of Waffle charts tends to blur the "percentage" precision below the integer limit anyway
  • and having a popover named "Labels" only for that seems overkill.

Screenshot 2021-11-24 at 17 25 38

While it is still experimental I may propose to get rid of the popover altogether and keep only the Legend one? Maybe we could workout a better way to expose the same feature, in the future, in a better context?

Probably only 1, 5 and 6 can be addressed in this PR as all the others belong more to the elastic-charts realm. What do you think about them?

warningMessages.push(
<FormattedMessage
id="xpack.lens.pie.smallValuesWarningMessage"
defaultMessage="The received data contains too small values that can be incorrectly visualized in the {shape} chart"
Copy link
Contributor

@mbondyra mbondyra Nov 24, 2021

Choose a reason for hiding this comment

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

@KOTungseth maybe you could help out with the copy for this message? The screenshot of how it's used is here

Copy link
Contributor

Choose a reason for hiding this comment

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

How about "The data contains small category values that could be incorrectly represented by the waffle chart. Try switching to table or treemap to better represent these values."

@alexwizp
Copy link
Contributor Author

@markov00 could you please review ☝️ 2,3,4 comments ? Can we do something about it?

Thanks @dej611 for your review 🍿

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@alexwizp alexwizp requested a review from flash1293 November 25, 2021 15:06
@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

Design question: Maybe the legend should be by default at the bottom? I think it look better for this chart. CC @MichaelMarcialis
Screenshot 2021-11-29 at 11 55 36

@mbondyra
Copy link
Contributor

Screenshot 2021-11-29 at 13 18 10

Can the setting from the left sight of legend be ever active? I cannot make it work?

@mbondyra
Copy link
Contributor

mbondyra commented Nov 29, 2021

Screenshot 2021-11-29 at 13 20 44

I cannot make sense of this legend vs tooltip information. The tooltip says there's 4 records in range 0.0$, but the legend says 0.0$ 0. Am I missing something or is there a bug?

Screenshot 2021-11-29 at 13 22 14

@mbondyra
Copy link
Contributor

mbondyra commented Nov 29, 2021

For some reason, the legend trims very short label: EUR to E..., but doesn't do the same with longer labels in another chart.
Screenshot 2021-11-29 at 13 25 14
Screenshot 2021-11-29 at 13 25 24

Also, when hovering over the legend item, I don't see the browser default tooltip (see screenshot three, this is the case for xy chart and it works
Screenshot 2021-11-29 at 13 57 37
)

@dej611
Copy link
Contributor

dej611 commented Nov 29, 2021

Design question: Maybe the legend should be by default at the bottom? I think it look better for this chart. CC @MichaelMarcialis Screenshot 2021-11-29 at 11 55 36

I like the side legend by default as it can show a larger the amount of categories, compared to the bottom one.

@ghudgins
Copy link
Contributor

CC @gvnmagni on the legend question too

@gvnmagni
Copy link

Hi all! In this case I agree with @dej611 , if we keep the legend on the side by default we might prevent problems with long list of elements. If isn't there something that I'm missing I'd suggest to keep it on the side

@alexwizp
Copy link
Contributor Author

@elasticmachine merge upstream

@markov00
Copy link
Member

markov00 commented Dec 1, 2021

@alexwizp I've just read the comments and the features and I like to comment on them:

  • related to @dej611 comments 2,4: the default sorting order is the one that comes from the data, you can specify your sorting order adding the following to the Partition layer configuration:
sortPredicate: ([, node1]: ArrayEntry, [, node2]: ArrayEntry) => {
  return node2.value - node1.value;
}

This will sort series by value, on both the legend and the chart. Please provide a dataset example if this sorting is not correctly applied on both legend and chart the same way.

  • can we limit the number of terms to a max of 10? 10 already saturate the number of colors and if we go further we start using the "desaturated" colors, that IMHO looks like disabled/hidden/not available geometries on the chart.
    If the user really needs such an extreme case, this chart should probably not be used, we should prevent this from happening, in particular, if we don't have enough colors to use. Let me ping @gvnmagni for a design opinion on that
    image

  • the legend issue: I saw that, can you please open an issue on the elastic-charts repo? We can take a look

@flash1293
Copy link
Contributor

flash1293 commented Dec 1, 2021

can we limit the number of terms to a max of 10? 10 already saturate the number of colors and if we go further we start using the "desaturated" colors, that IMHO looks like disabled/hidden/not available geometries on the chart.

I think this is what the warning is for - it explains to the user that the cardinality is too high for this chart type. It's very difficult to completely prevent the user from doing this because in a lot of situations the terms depend dynamically on the data and this case could happen on the dashboard as well. We have the same issue in a lot of places (e.g. a pie chart with hundreds of slices, a bar chart with lots of series, you name it), I'm not a fan of implementing some special handling for this chart type but rather think through this holistically in a separate project.

There is no single place to prevent the user from entering a number larger than 10 to not have this happening unfortunately, it's more complicated than that.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 683 685 +2

Async chunks

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

id before after diff
lens 968.0KB 971.7KB +3.7KB

History

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

cc @alexwizp

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested locally in Safari.
The sorting behaviour is much better, but there are still some edge cases which I suspect may be an elastic-chart bug:

Screenshot 2021-12-01 at 17 14 47

But that's not a blocker for this PR.

A suggestion as a follow up PR would be to have a way to not display values on legend, but just labels. Sometimes it's ok to have just a rough idea of quantities without knowing the exact amount.

Screenshot 2021-12-01 at 17 36 50

Also this one is not a blocker for the PR.

@MichaelMarcialis
Copy link
Contributor

Design question: Maybe the legend should be by default at the bottom? I think it look better for this chart. CC @MichaelMarcialis
Screenshot 2021-11-29 at 11 55 36

@mbondyra: Good question. My initial instinct is to keep it defaulted to right alignment for two reasons: 1) it's consistent with the default legend alignment of all other visualization types in Lens, and 2) the bottom aligned legend requires scrolling when there is more than four legend items (a fact that could be missed or annoying to users when crafting their visualization).

CCing @gvnmagni as well, in case he has other thoughts.

@gvnmagni
Copy link

gvnmagni commented Dec 2, 2021

@MichaelMarcialis yes, agree with you. Legend on the side would still be my preferred choice, it is just way more easier to handle and more consistent with all the rest.

@markov00 @flash1293 agree with both of you, unfortunately. Until our users are not educated enough (or guided enough) to make the right choice I'm afraid the only thing we can do is to signal them that they are doing something that could be done better with a different chart. I'm not sure what we use typically so point out things like this (a warning? a popover message?) but probably is the best thing to do. When we have too many categories we should say to the user ("This chart is not recommended for this number of categories, you should try with a stacked barchart" or something similar)

@alexwizp alexwizp merged commit 3cd176f into elastic:main Dec 2, 2021
@ghudgins ghudgins mentioned this pull request Dec 7, 2021
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
* [WIP][Lens] Waffle visualization type

Closes:  elastic#107059

* add showExtraLegend for waffle

* add tests

* resolved 1 and 5

* resolved 6

* add sortPredicate for waffle chart type

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Waffle visualization type