-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Conversation
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. 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 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. |
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. |
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:
|
There was a problem hiding this 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
@monfera thanx a lot for your feedback.
|
@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? |
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. |
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:
|
@elasticmachine merge upstream |
For clarity, this is what I mean by the adaptive text size (some storybook examples in |
There was a problem hiding this 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!
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 |
There was a problem hiding this 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: { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
const legendPosition = useMemo(() => visParams.legendPosition ?? Position.Right, [ | ||
visParams.legendPosition, | ||
]); |
There was a problem hiding this comment.
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%"> |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))}...`; |
There was a problem hiding this comment.
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 :)
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> { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@elasticmachine merge upstream |
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:
Maybe we can consider the use of left/center and right/center legend aligment too at some point. |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: |
…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
…#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>
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.
New settings:
Inside
and disabled.Extra
Legacy charts
switch is moved to the visualizations plugin.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.