[PieVis] PartitionVis integration to Lens. #123937

Kuznietsov · 2022-01-27T15:04:18Z

Summary

Completes part of #121725 and #95902.

Removed lens_pie expression and lens_pie_renderer.
Removed styles, related to lens_pie_renderer.
Replaced lens_pie with pieVis/mosaicVis/treemapVis/waffleVis expressions.
Updated tests to reflect visualization changes.

# Conflicts: # src/plugins/chart_expressions/expression_pie/common/expression_functions/pie_vis_function.test.ts

…ptation

…ptation # Conflicts: # src/plugins/chart_expressions/expression_pie/common/expression_functions/pie_labels_function.ts

…ptation

elasticmachine · 2022-02-08T08:17:37Z

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

flash1293 · 2022-02-09T13:36:18Z

This is mostly working fine, but I noticed a weird bug when changing the order of bucket dimensions:

Doesn't happen on main

Kuznietsov · 2022-02-10T08:25:57Z

@elasticmachine merge upstream

flash1293

Tested and didn't notice any problems except for the issue with changing/reordering dimensions.

@Kunzetsov I tested around a bit more and it's not only possible to run into this case on re-ordering the dimensions via drag and drop - it also happens e.g. on dragging in a new field.

I'm not feeling comfortable merging the PR this way even if we start working on a fix right away as it will break currently working behavior in subtle ways. For this PR, let's deploy a band-aid fix to be on the safe side:

In the pie expression building logic, use this piece of code to get the operations:

  const operations = datasource
    .getTableSpec()
    .map((col) => col.columnId)
    .filter((columnId) => layer.groups.includes(columnId))
    .map((columnId) => ({ columnId, operation: datasource.getOperationForColumnId(columnId) }))
    .filter((o): o is { columnId: string; operation: Operation } => !!o.operation);

It will ensure the order of the table is respected

Kuznietsov · 2022-02-10T11:09:09Z

Tested and didn't notice any problems except for the issue with changing/reordering dimensions.

@Kunzetsov I tested around a bit more and it's not only possible to run into this case on re-ordering the dimensions via drag and drop - it also happens e.g. on dragging in a new field.

I'm not feeling comfortable merging the PR this way even if we start working on a fix right away as it will break currently working behavior in subtle ways. For this PR, let's deploy a band-aid fix to be on the safe side:

In the pie expression building logic, use this piece of code to get the operations:
  const operations = datasource
    .getTableSpec()
    .map((col) => col.columnId)
    .filter((columnId) => layer.groups.includes(columnId))
    .map((columnId) => ({ columnId, operation: datasource.getOperationForColumnId(columnId) }))
    .filter((o): o is { columnId: string; operation: Operation } => !!o.operation);
It will ensure the order of the table is respected

As we've decided in the private conversation, I'll create the fix in a separate PR, merge it to the main, and only after that we'll come back to the current one.

mbondyra

EDIT: I read Joe's comment after testing, it's the same issue, sorry for the noise I started some initial testing and noticed a bug on Lens with reordering. When reordering two dimensions, they get duplicated (in the chart, not in the dimension panel)

Kuznietsov · 2022-02-11T07:19:59Z

EDIT: I started reading Joe's comment after testing, it's the same issue, sorry for the noise I started some initial testing and noticed a bug on Lens with reordering. When reordering two dimensions, they get duplicated (in the chart, not in the dimension panel)

@mbondyra, no problem)

Kuznietsov · 2022-02-11T07:20:54Z

@flash1293, @mbondyra, I'm close to finishing the fix of the current behavior at this PR: #125336

mbondyra · 2022-02-11T13:26:17Z

...ins/chart_expressions/expression_partition_vis/public/components/visualization_noresults.tsx

+export const VisualizationNoResults: FC<Props> = ({ hasNegativeValues = false, chartType }) => {
+  if (hasNegativeValues) {
+    return (
+      <EuiText


EmptyPlaceholder component takes icon, message and dataTestSubj parameters so I think you can replace this code with using it directly (you'd have to add iconType or something like this though). What do you think?

@mbondyra, updated the code, according to your proposition. Thanks for the suggestion)

# Conflicts: # x-pack/plugins/lens/public/pie_visualization/to_expression.ts # x-pack/plugins/lens/public/pie_visualization/visualization.tsx

Kuznietsov · 2022-02-11T14:28:59Z

@flash1293, tested on this branch. Working as expected.

Screen.Recording.2022-02-11.at.16.25.30.mov

mbondyra

Tested on Chrome and works as expected. I found some minor issues while testing with pie not related to this feature, but reproduced all on main branch so I'll just create new issues. Approved, code LGTM 👌🏼 Amazing job!

Kuznietsov · 2022-02-11T14:46:39Z

@elastic/security-asset-management @elastic/kibana-design, could you, please, review this PR. Thanks.

Kuznietsov · 2022-02-11T16:16:25Z

@elasticmachine merge upstream

Kuznietsov · 2022-02-14T08:15:17Z

@elasticmachine merge upstream

Kuznietsov · 2022-02-14T08:16:08Z

@elastic/security-asset-management, could you, please, review this PR?) Thanks)

Kuznietsov · 2022-02-14T10:08:37Z

@elasticmachine merge upstream

Kuznietsov · 2022-02-14T12:10:26Z

@elasticmachine merge upstream

kibana-ci · 2022-02-14T13:44:25Z

💛 Build succeeded, but was flaky

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`expressionPartitionVis`	49	56	+7
`lens`	744	734	-10
total			-3

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`	286	288	+2
`lens`	316	343	+27
total			+29

Async chunks

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

id	before	after	diff
`expressionPartitionVis`	42.5KB	47.0KB	+4.5KB
`lens`	1.0MB	1.0MB	-5.9KB
total			-1.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`lens`	44	41	-3

Page load bundle

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

id	before	after	diff
`charts`	58.3KB	58.4KB	+67.0B
`expressionPartitionVis`	20.1KB	20.1KB	+15.0B
`lens`	43.4KB	42.5KB	-901.0B
total			-819.0B

Unknown metric groups

API count

id	before	after	diff
`charts`	319	321	+2
`lens`	365	392	+27
total			+29

History

💔 Build #23808 failed f492215
💔 Build #23770 failed aa7db6a
💛 Build #23562 was flaky 0f144c4
💔 Build #23512 failed ed5fe34
💚 Build #23043 succeeded 877e39b

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

cc @Kunzetsov

patrykkopycinski

Asset management LGTM

Kuznietsov and others added 30 commits December 17, 2021 18:11

Moved pie_vis to the other.

d13df0b

Added semicolon.

32a2a81

Fixed check errors.

c5c41a6

Removed not used types.

4c40252

Changed docs.

6ef8429

Fixed labels.

282c017

Fixed more translation labels.

b01e118

Merge branch 'main' into charts_expressions-pie

3231f72

# Conflicts: # src/plugins/chart_expressions/expression_pie/common/expression_functions/pie_vis_function.test.ts

Removed own types and replaced by expression_pie.

e54c816

Changed types of buildExpressionFunction.

a363fb3

Added limits and extraPublicDir.

7faa2b6

Fixed i18n checks.

604f497

Fixes.

1f9d54c

More fixes.

90dd2ee

Fixed translations checks.

c300657

More translation fixes.

457e4f8

Added displayName and help for renderer.

17d6c65

Removed as statement.

679e11c

Merge branch 'main' into charts_expressions-pie

2160608

Replaced data.fieldFormats with fieldFormats.

e83b93d

Added support of visdimension.

37aacf8

Fixed types of ExpressionValueVisDimension.

2e8cf1b

Merge branch 'main' into charts_expressions-pie-adaptation

de2a7a8

Merge branch 'charts_expressions-pie' into charts_expressions-pie-ada…

5aacf11

…ptation

Fixed Arguments type of pie_labels.

4662d77

Merge branch 'charts_expressions-pie' into charts_expressions-pie-ada…

6f4028e

…ptation # Conflicts: # src/plugins/chart_expressions/expression_pie/common/expression_functions/pie_labels_function.ts

Merge branch 'main' into charts_expressions-pie

25c9e6a

Merge branch 'charts_expressions-pie' into charts_expressions-pie-ada…

ca687ef

…ptation

Merge branch 'main' into charts_expressions-pie-adaptation

e2fa8fc

Fixed wrong behavior of pie_vis without args.

ec12662

Merge branch 'main' into charts_expressions-pie-lens-integration

877e39b

flash1293 reviewed Feb 10, 2022

View reviewed changes

Kuznietsov added the blocked label Feb 10, 2022

mbondyra reviewed Feb 10, 2022

View reviewed changes

mbondyra reviewed Feb 11, 2022

View reviewed changes

Merge branch 'main' into charts_expressions-pie-lens-integration

3c080e3

# Conflicts: # x-pack/plugins/lens/public/pie_visualization/to_expression.ts # x-pack/plugins/lens/public/pie_visualization/visualization.tsx

mbondyra approved these changes Feb 11, 2022

View reviewed changes

Refactored visualization_noresults.

ed5fe34

Kuznietsov removed the blocked label Feb 11, 2022

MichaelMarcialis approved these changes Feb 11, 2022

View reviewed changes

Merge branch 'main' into charts_expressions-pie-lens-integration

0f144c4

Merge branch 'main' into charts_expressions-pie-lens-integration

aa7db6a

Merge branch 'main' into charts_expressions-pie-lens-integration

f492215

Merge branch 'main' into charts_expressions-pie-lens-integration

e6fb686

patrykkopycinski approved these changes Feb 14, 2022

View reviewed changes

Kuznietsov merged commit 54de36f into elastic:main Feb 14, 2022

[PieVis] PartitionVis integration to Lens. #123937

[PieVis] PartitionVis integration to Lens. #123937

Uh oh!

Conversation

Kuznietsov commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Feb 8, 2022

Uh oh!

flash1293 commented Feb 9, 2022

Uh oh!

Kuznietsov commented Feb 10, 2022

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Kuznietsov commented Feb 10, 2022

Uh oh!

mbondyra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kuznietsov commented Feb 11, 2022

Uh oh!

Kuznietsov commented Feb 11, 2022

Uh oh!

mbondyra Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kuznietsov Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

Kuznietsov commented Feb 11, 2022

Uh oh!

mbondyra left a comment

Choose a reason for hiding this comment

Uh oh!

Kuznietsov commented Feb 11, 2022

Uh oh!

Kuznietsov commented Feb 11, 2022

Uh oh!

Kuznietsov commented Feb 14, 2022

Uh oh!

Kuznietsov commented Feb 14, 2022

Uh oh!

Kuznietsov commented Feb 14, 2022

Uh oh!

Kuznietsov commented Feb 14, 2022

Uh oh!

kibana-ci commented Feb 14, 2022

💛 Build succeeded, but was flaky

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

Uh oh!

patrykkopycinski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Kuznietsov commented Jan 27, 2022 •

edited

Loading

mbondyra left a comment •

edited

Loading

mbondyra Feb 11, 2022 •

edited

Loading