[PieVis] Lens adaptation. #122420
[PieVis] Lens adaptation. #122420
Conversation
# Conflicts: # src/plugins/chart_expressions/expression_pie/common/expression_functions/pie_vis_function.test.ts
…ptation # Conflicts: # src/plugins/chart_expressions/expression_pie/common/expression_functions/pie_labels_function.ts
|
@elastic/kibana-vis-editors @elastic/kibana-vis-editors-external, could you, please, review the current PR?) Thanks) |
# Conflicts: # src/plugins/visualizations/server/embeddable/make_visualize_embeddable_factory.ts # src/plugins/visualizations/server/migrations/visualization_saved_object_migrations.ts
Kuznietsov
added
v8.2.0
impact:medium
|
@elasticmachine merge upstream |
|
@Kunzetsov ignore my above comment. My kibana wanted the nuclear command to work properly 😓 |
There was a problem hiding this comment.
This looks good to me.
I tested the visualize pie and I cant find any regression. I also tested the canvas expressions. I haven't covered all scenarios on the latter but it seems to work fine.
@flash1293 when you have some time, do you want to also have a look? Just to be sure that I haven't missed anything :)
# Conflicts: # src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.tsx # src/plugins/chart_expressions/expression_partition_vis/public/utils/get_color_picker.tsx # src/plugins/chart_expressions/expression_partition_vis/public/utils/index.ts
|
@flash1293, could you, please, review the current PR? Thanks a lot) Your review is the only, which is required for now) |
|
@Kunzetsov On it, this is my main task for today! |
There was a problem hiding this comment.
Not a big thing, but I noticed the pie vis doesn't prevent you from using both row and column split which won't work right:
In this example with a table with two split dimensions I'm mapping them to row and column split but only the column split is shown.
There are no blockers for me except for the migration question which I would like to resolve (also cc @stratoula ). But I don't have a super strong opinion about it
|
|
||
| const sortPredicateSaveSourceOrder: SortPredicatePureFn = | ||
| () => | ||
| ([, node1], [, node2]) => { |
There was a problem hiding this comment.
I didn't test, but is Predicate.DataIndex from the elastic-charts plugin doing this as well? Used hereL
| truncate: number | null; | ||
| }; | ||
| } | ||
| legendDisplay: LegendDisplay; |
There was a problem hiding this comment.
While testing this I realized you can't turn the legend on in vislib pie - the button in the bottom left is missing. This is also happening in 8.0. Just a note, I don't think we have to solve on this PR
| if (visState && visState.type === 'pie') { | ||
| const { addLegend, labels, ...restParams } = visState.params; | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| const { last_level, ...restLabels } = labels ?? {}; |
There was a problem hiding this comment.
As last_level is deprecated anyway, does it make sense to run a migration for it right now? It seems like we can easily map it to lastLevel on the expression here:
That way we wouldn't have to touch the saved object. What do you think?
There was a problem hiding this comment.
@flash1293, @stratoula, addressed this issue at the current commit: 5f7ceaf
There was a problem hiding this comment.
@flash1293, we couldn't do that, because to_ast logic is related to the actual shape of the expression arguments. If we would override it, we'd lose the connection between vis_types and the expression. I've tried and come to the idea, it is better to leave it as it has been before.
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @Kunzetsov |
## Summary Closes #159942 If the height of a partition chart exceeds 1000px paddings are added, reducing the chart size. This is caused due to this piece of code #122420 This was added for the aggbased editor to reduce a bit the pie size (otherwise it was taking the full container size and the pie was huge) Although we want this, we don't want this to be applied in dashboards or lens editor. This PR is fixing this by adding the paddings only on the agg based editor level In agg based editor <img width="651" alt="image" src="https://github.com/elastic/kibana/assets/17003240/48ac6fdd-43e3-46f5-8818-d40334678fce"> Dashboard with very tall treemap, no paddings <img width="933" alt="image" src="https://github.com/elastic/kibana/assets/17003240/8787d6ab-887c-4c8d-8419-2c2d5659f2c1"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Summary
Completes part of #121725, #95902 and #101377.
Performed changes
chart_expressions/expression_pietochart_expressions/expression_partition_vis.pieVisexpression renderer topartitionVis.pie/donut/mosaic/treemap/waffleconfiguration from Lens and VisEditors bypartitionVisrenderer.mosaic.treemap.pie/donut/mosaic/treemapviapartitionVisrenderer.