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

[Pie] New implementation of the vislib pie chart with es-charts #83929

Merged
merged 193 commits into from
Jun 3, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Nov 20, 2020

Summary

closes #16746 closes #82151 closes #62204 closes #50993 closes #34050 closes #16931 closes #15184 closes #57802

Part of #56143. It replaces the vislib implementation of the pie chart with the elastic-charts Partition.

Screenshot 2020-12-22 at 10 58 18 AM

image

New settings:

  • Nested legend. The users can choose if they want a detailed legend or not
  • Uses the new palette service. User can choose the palette to use. The old charts default on the legacy palette while the new ones use the default one. All the sample data pie charts have been modified to use the default EUI palette.
  • Distinct color per slice. It defaults to false for the newly created charts but gives the users the possibility to switch it on if they want the same experience as vislib. When the switch is on, the slices with the same value take the same color. For the already created visualizations it will default to true in order to not introduce a big styling change to our users.
  • Label position: Exactly like Lens user can choose the position of the labels inside/outside the chart. In case of small multiples this value is always set to Inside and disabled.
  • Values format: Users can select the format of the values, so they can choose between percent or values. (On the vislib chart values are only depicted as percentages)
  • Show legend, although there is the button on the bottom left to display/hide the legends, I think that it is not so obvious for the users to understand what it does so I have added this extra switch to change the visibility from here too. The default value will be false.
  • Maximal decimal places for percent: Defines the number of decimals that will appear on the slice value. It is enabled only in case of percent format
    image

Extra

  • The Legacy charts switch is moved to the visualizations plugin.
  • Dashboard syncColors is supported
  • Replaced the visJson blob with explicit params
  • Updated the sample pie viz with the default EUI palette

Important note

The tooltip used is the elastic-charts one. In order to have a more detailed tooltip, we should wait to be implemented on elastic-charts side elastic/elastic-charts#615

Overwrite Colors

The users can overwrite the colors of the inner layer only for all palettes when the Distinct colors per slice switch is off. If it is on, the users can overwrite all the colors of all the layers. If the slices have the same value/color the color is overwritten in all these slices.

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula changed the title [WIPi] Vislib pie replacement [WIP] Vislib pie replacement Nov 20, 2020
@stratoula
Copy link
Contributor Author

stratoula commented Nov 20, 2020

This PR is not ready for sure and has many bugs (so please ignore them 😄 ) I have just created it in order to be able to discuss the color picker functionality of the pie chart based on this issue #62204. I have used the new palette service and the outer layers are coloured based on the inner layer.

Screenshot 2020-11-20 at 6 15 30 PM

So if the user changes the colors from the legendPicker of the inner layer, all the colors of the outer layers will nicely be adjusted.

But what is the behavior we want if the user wants to change the colors of the outer layer? If you enable the nested legend switch on this PR and change the color of false in the above example I change all color of all the false occurrences (this is also what the vislib pie does right now).

Screenshot 2020-11-20 at 6 17 56 PM

But is this what we want? I think we should sync on that in order to do the same with what Lens wants to do in the future.
I would love your feedback on that 🙂 cc @flash1293 @timroes @markov00 @nickofthyme

@flash1293
Copy link
Contributor

Good question @stratoula - I think in Lens we decided for the current color scheme to avoid introducing too many colors (something the vislib pie chart does not do well). If the user is specifically picking it I can see the use case, but I'm still not sure whether we should allow it. @markov00 s feedback will definitely be more helpful than mine.

@monfera
Copy link
Contributor

monfera commented May 31, 2021

Fantastic work in this PR!

A few comments relating to the user experience from a dataviz viewpoont. Not sure if they're fair play for this PR, though it may be a good point of time to consider:

  1. Upon creation, the pie chart fills the entire screen, it is gigantic

  2. Also, it's not a pie, it's a donut, which is inferior even to the pie chart

  3. It shows a single "slice"—count—which isn't a super useful first breakdown
    image

  4. The largest slice falls to the left of the 12 o'clock mark, which is often sensible thing to do except when there's an "other" slice; for this reason, and for vislib compatibility, the specialFirstInnermostSector could be set to false to avoid this

  5. The pie sorting PR enables the placement of the "Other" slice to the end, even if it's not the smallest slice
    image

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

It's an epic and ambitious PR, great job Stratoula!

Asking for some changes and manual developer testing as eg. the Options don't seem to work, and some other things eg. slice order we discussed

@stratoula
Copy link
Contributor Author

@monfera thanx a lot for your feedback.

  1. As I have also mentioned here I cant find any way to resize the pie chart without changing the legend position. Any ideas from your side?
  2. This is how it works now, we don't want to change this behavior for now
  3. I agree but same as above, this is how it works right now
  4. I will do that, thanx!
  5. I can add Other to the end, I need to check how vislib behaved though
    6 & 7 & 8 & 9 & 10 I can't replicate them and they should work because I have tested them multiple times. Every time you change the options on the editor you should hit the Update button, otherwise they are not updated.

@monfera monfera self-requested a review May 31, 2021 15:21
@stratoula
Copy link
Contributor Author

stratoula commented Jun 1, 2021

@monfera I have addressed the 4 and 5. Now it works exactly as vislib. Thank you a lot for that. 8ea3f60
About the size, do you have any suggestions? If I resize the container, the legend loses its position and we don't want this. Can I resize only the chart without affecting the position?
Elastic

@monfera
Copy link
Contributor

monfera commented Jun 1, 2021

@stratoula thank you! Would you want a reasonably sized pie chart (ie. a diameter that's a fraction of the full screen height, maybe some hundreds of pixels) associated with a legend that's still put on the very top / very right? In this case, the legend will be farther away from the chart than needed. Is it your intent, or do I misunderstand?

@stratoula
Copy link
Contributor Author

Yes @monfera because otherwise, pie chart will be displayed differently from vislib and all the other classic visualizations.

As you can in the old implementation, the legend is set to top right of the container.
image

@monfera
Copy link
Contributor

monfera commented Jun 2, 2021

Most likely not the result of this PR, but maybe you know of some history: the Legend position and Show legend fields are flipped:
image

@monfera
Copy link
Contributor

monfera commented Jun 2, 2021

Thank you @stratoula this PR represents a great amount of work on your part and though every change has the possibility of regressions, it brings in multi-row, in-slice labels, which are better for readability than the more distant linked labels and esp. the legends. A couple of improvements for eventual consideration:

  • variable font size, ie. similar to how most of the elastic-charts storyboard shows, it helps readability of slice names (they don't help of course with very small slices, though the ideal pie chart has few, therefore mostly large slices only)
  • the Z-shaped (grid) tiling of pie charts (eg. narrow the window here to see) which is often better than the other two chart options, row and column

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@monfera
Copy link
Contributor

monfera commented Jun 2, 2021

For clarity, this is what I mean by the adaptive text size (some storybook examples in elastic-charts), both the minimum and maximum font size are configurable, the rest is automatic. If it's not considered for Visualize, could still represent an advance in Lens
image

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Couldn't find any issues, LGTM.

Great work, this is getting rid of a large part of legacy code!

@stratoula
Copy link
Contributor Author

With @markov00 's help, we managed to resize the chart in order to not appear so huge on the editor. I chose a max size of 900px which seems to me that is closer to the vislib sizes.
image

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM, I've tested it locally on chrome and did another quicker code review.
I've left few comments, nothing major that needs to resolved in this PR (except the snake_case variable in the expression arguments if not actually required in that case)
Great work here Stratoula!

}),
default: 2,
},
last_level: {
Copy link
Member

Choose a reason for hiding this comment

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

Why this argument is written in snake_case instead of camelCase?
It looks like an introduced argument so probably we should keep it consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used by vislib and it was already defined like that so I can't change it without a migration script. But what I can do is to fix it at least on the expression level.

Comment on lines 225 to 227
const legendPosition = useMemo(() => visParams.legendPosition ?? Position.Right, [
visParams.legendPosition,
]);
Copy link
Member

Choose a reason for hiding this comment

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

nit: probably we should not use useMemo with this very simple code. we should leverage useMemo only for specific complex functions

showLegend={showLegend}
legendPosition={legendPosition}
/>
<Chart size="100%">
Copy link
Member

Choose a reason for hiding this comment

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

I've created an example on how we can solve this by using the outerSizeRatio or the margins
https://codesandbox.io/s/youthful-curran-egqkv?file=/src/App.tsx

partitionLayout: PartitionLayout.sunburst,
fontFamily: chartTheme.barSeriesStyle?.displayValue?.fontFamily,
outerSizeRatio: 1,
specialFirstInnermostSector: false,
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not wrong this should be false only if the other bucket is enabled. @monfera correct me if I'm wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to have it false in order to work like vislib.

if (visParams.labels.truncate && formattedLabel.length <= visParams.labels.truncate) {
return formattedLabel;
} else {
return `${formattedLabel.slice(0, Number(visParams.labels.truncate))}...`;
Copy link
Member

Choose a reason for hiding this comment

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

Better use the unicode char to save 2 chars :)

Suggested change
return `${formattedLabel.slice(0, Number(visParams.labels.truncate))}...`;
return `${formattedLabel.slice(0, Number(visParams.labels.truncate))}\u2026`;

@@ -45,32 +47,32 @@ export function VisualizeChartPageProvider({ getService, getPageObjects }: FtrPr
/**
* Is new charts library enabled and an area, line or histogram chart exists
*/
private async isVisTypeXYChart(): Promise<boolean> {
public async isNewLibraryChart(chartSelector: string): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

update the comment to include also the pie charts
side note: having a single setting for the legacy vislib means that when a user discovers a bug on the pie or xys solvable switching back to the legacy implementation, it causes going back also on every charts: both pie and xys, causing a major change in every chart. Probably is worth chatting about having different settings for each legacy feature to minimize changes when switch back

Copy link
Member

Choose a reason for hiding this comment

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

🔝 this actually is also something that I like to discuss before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Marco this is correct. We have already discussed it with @timroes and we decided to go with one setting for now.

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@monfera
Copy link
Contributor

monfera commented Jun 3, 2021

With @markov00 's help, we managed to resize the chart in order to not appear so huge on the editor. I chose a max size of 900px which seems to me that is closer to the vislib sizes

Stratoula's image example here

Post-PR comment, or likely, not even for vislib, though maybe interesting for Lens (cc @ghudgins): there may be other forces at play that we could discuss, but in general, striving for an increased distance between the chart and the legend is an anti-goal, because it makes the user's life harder, eg. long distance or more fragmented saccades; more erasure in the short term memory. So, all things being equal, it'd be neater to more closely align the legend with the chart.

There are of course forces that might counteract this:

  • expectation of consistency in legend placement (though it's a moot point, because ultimately we shouldn't show a less than huge pie chart in a huge container, and we shouldn't show huge pie charts at all)
  • if the legend has very many items (which is best to avoid anyway), then it makes sense to start on the very top, to maximize the area (number of legend rows)

Maybe we can consider the use of left/center and right/center legend aligment too at some point.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypePie - 50 +50
visTypeVislib 205 204 -1
total +49

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
charts 155 159 +4

Async chunks

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

id before after diff
charts 56.5KB 56.2KB -342.0B
visTypePie - 74.6KB +74.6KB
visTypeVislib 569.2KB 564.7KB -4.5KB
visualizations 101.2KB 101.2KB +27.0B
total +69.8KB

Page load bundle

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

id before after diff
charts 87.4KB 88.5KB +1.1KB
visTypePie - 20.1KB +20.1KB
visTypeVislib 33.4KB 32.3KB -1.1KB
visTypeXy 63.3KB 63.3KB +49.0B
visualizations 55.1KB 55.5KB +419.0B
total +20.5KB
Unknown metric groups

API count

id before after diff
charts 186 190 +4

async chunk count

id before after diff
visTypePie - 2 +2
visTypeVislib 5 4 -1
total +1

History

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

@stratoula stratoula merged commit a9a9013 into elastic:master Jun 3, 2021
stratoula added a commit to stratoula/kibana that referenced this pull request Jun 3, 2021
…tic#83929)

* es lint fix

* Add formatter on the buckets labels

* Config the new plugin, toggle tooltip

* Aff filtering on slice click

* minor fixes

* fix eslint error

* use legacy palette for now

* Add color picker to legend colors

* Fix ts error

* Add legend actions

* Fix bug on Color Picker and remove local state as it is unecessary

* Fix some bugs on colorPicker

* Add setting for the user to select between the legacy palette or the eui ones

* small enhancements, treat empty labels with (empty)

* Fix color picker bugs with multiple layers

* fixes on internationalization

* Create migration script for pie chart and legacy palette

* Add unit tests (wip) and a small refactoring

* Add unit tests and move some things to utils, useMemo and useCallback where it should

* Add jest config file

* Fix jest test

* fix api integration failure

* Fix to_ast_esaggs for new pie plugin

* Close legendColorPicker popover when user clicks outside

* Fix warning

* Remove getter/setters and refactor

* Remove kibanaUtils from pie plugin as it is not needed

* Add new values to the migration script

* Fix bug on not changing color for expty string

* remove from migration script as they don't need it

* Fix editor settings for old and new implementation

* fix uistate type

* Disable split chart for the new plugin for now

* Remove temp folder

* Move translations to the pie plugin

* Fix CI failures

* Add unit test for the editor config

* Types cleanup

* Fix types vol2

* Minor improvements

* Display data on the inspector

* Cleanup translations

* Add telemetry for new editor pie options

* Fix missing translation

* Use Eui component to detect click outside the color picker popover

* Retrieve color picker from editor and syncColors on dashboard

* Lazy load palette service

* Add the new plugin to ts references, fix tests, refactor

* Fix ci failure

* Move charts library switch to vislib plugin

* Remove cyclic dependencies

* Modify license headers

* Move charts library switch to visualizations plugin

* Fix i18n on the switch moved to visualizations plugin

* Update license

* Fix tests

* Fix bugs created by new charts version

* Fix the i18n switch problem

* Update the migration script

* Identify if colorIsOverwritten or not

* Small multiples, missing the click event

* Fixes the UX for small multiples part1

* Distinct colors per slice implementation

* Fix ts references problem

* Fix some small multiples bugs

* Add unit tests

* Fix ts ref problem

* Fix TS problems caused by es-charts new version

* Update the sample pie visualizations with the new eui palette

* Allows filtering by the small multiples value

* Apply sortPredicate on partition layers

* Fix vilib test

* Enable functional tests for new plugin

* Fix some functional tests

* Minor fix

* Fix functional tests

* Fix dashboard tests

* Fix all dashboard tests

* Apply some improvements

* Explicit params instead of visConfig Json

* Fix i18n failure

* Add top level setting

* Minor fix

* Fix jest tests

* Address PR comments

* Fix i18n error

* fix functional test

* Add an icon tip on the distinct colors per slice switch

* Fix some of the PR comments

* Address more PR comments

* Small fix

* Functional test

* address some PR comments

* Add padding to the pie container

* Add a max width to the container

* Improve dashboard functional test

* Move the labels expression function to the pie plugin

* Fix i18n

* Fix functional test

* Apply PR comments

* Do not forget to also add the migration to them embeddable too :D

* Fix distinct colors for IP range layer

* Remove console errors

* Fix small mulitples colors with multiple layers

* Fix lint problem

* Fix problems created from merging with master

* Address PR comments

* Change the config in order the pie chart to not appear so huge on the editor

* Address PR comments

* Change the max percentage digits to 4

* Change the max size to 1000

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	.github/CODEOWNERS
#	packages/kbn-optimizer/limits.yml
#	test/functional/apps/visualize/_pie_chart.ts
stratoula added a commit that referenced this pull request Jun 4, 2021
…#83929) (#101292)

* [Pie] New implementation of the vislib pie chart with es-charts (#83929)

* es lint fix

* Add formatter on the buckets labels

* Config the new plugin, toggle tooltip

* Aff filtering on slice click

* minor fixes

* fix eslint error

* use legacy palette for now

* Add color picker to legend colors

* Fix ts error

* Add legend actions

* Fix bug on Color Picker and remove local state as it is unecessary

* Fix some bugs on colorPicker

* Add setting for the user to select between the legacy palette or the eui ones

* small enhancements, treat empty labels with (empty)

* Fix color picker bugs with multiple layers

* fixes on internationalization

* Create migration script for pie chart and legacy palette

* Add unit tests (wip) and a small refactoring

* Add unit tests and move some things to utils, useMemo and useCallback where it should

* Add jest config file

* Fix jest test

* fix api integration failure

* Fix to_ast_esaggs for new pie plugin

* Close legendColorPicker popover when user clicks outside

* Fix warning

* Remove getter/setters and refactor

* Remove kibanaUtils from pie plugin as it is not needed

* Add new values to the migration script

* Fix bug on not changing color for expty string

* remove from migration script as they don't need it

* Fix editor settings for old and new implementation

* fix uistate type

* Disable split chart for the new plugin for now

* Remove temp folder

* Move translations to the pie plugin

* Fix CI failures

* Add unit test for the editor config

* Types cleanup

* Fix types vol2

* Minor improvements

* Display data on the inspector

* Cleanup translations

* Add telemetry for new editor pie options

* Fix missing translation

* Use Eui component to detect click outside the color picker popover

* Retrieve color picker from editor and syncColors on dashboard

* Lazy load palette service

* Add the new plugin to ts references, fix tests, refactor

* Fix ci failure

* Move charts library switch to vislib plugin

* Remove cyclic dependencies

* Modify license headers

* Move charts library switch to visualizations plugin

* Fix i18n on the switch moved to visualizations plugin

* Update license

* Fix tests

* Fix bugs created by new charts version

* Fix the i18n switch problem

* Update the migration script

* Identify if colorIsOverwritten or not

* Small multiples, missing the click event

* Fixes the UX for small multiples part1

* Distinct colors per slice implementation

* Fix ts references problem

* Fix some small multiples bugs

* Add unit tests

* Fix ts ref problem

* Fix TS problems caused by es-charts new version

* Update the sample pie visualizations with the new eui palette

* Allows filtering by the small multiples value

* Apply sortPredicate on partition layers

* Fix vilib test

* Enable functional tests for new plugin

* Fix some functional tests

* Minor fix

* Fix functional tests

* Fix dashboard tests

* Fix all dashboard tests

* Apply some improvements

* Explicit params instead of visConfig Json

* Fix i18n failure

* Add top level setting

* Minor fix

* Fix jest tests

* Address PR comments

* Fix i18n error

* fix functional test

* Add an icon tip on the distinct colors per slice switch

* Fix some of the PR comments

* Address more PR comments

* Small fix

* Functional test

* address some PR comments

* Add padding to the pie container

* Add a max width to the container

* Improve dashboard functional test

* Move the labels expression function to the pie plugin

* Fix i18n

* Fix functional test

* Apply PR comments

* Do not forget to also add the migration to them embeddable too :D

* Fix distinct colors for IP range layer

* Remove console errors

* Fix small mulitples colors with multiple layers

* Fix lint problem

* Fix problems created from merging with master

* Address PR comments

* Change the config in order the pie chart to not appear so huge on the editor

* Address PR comments

* Change the max percentage digits to 4

* Change the max size to 1000

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	.github/CODEOWNERS
#	packages/kbn-optimizer/limits.yml
#	test/functional/apps/visualize/_pie_chart.ts

* Fix functional test

* Revert change - backport missing

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Pie Chart Pie chart visualization feature release_note:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet