[XY axis] Improve expression with explicit params #98897

VladLasitsa · 2021-04-30T10:55:39Z

Summary

Right now the vis_type_xy is using a JSON visConfig which is not ideal for other teams to reuse it.
We should remove the JSON blob and use explicit params instead. In this PR I removed visConfig and use all params from visConfig as args in expression function defenition for xy plugin. Also I wrote some new expression function defenition for params which have complex object structure.

VladLasitsa · 2021-04-30T15:21:43Z

@elasticmachine merge upstream

VladLasitsa · 2021-05-12T06:12:41Z

@elasticmachine merge upstream

elasticmachine · 2021-05-12T08:00:56Z

Pinging @elastic/kibana-app (Team:KibanaApp)

stratoula · 2021-05-13T09:28:22Z

@elasticmachine merge upstream

stratoula

@VladLasitsa this looks pretty good. Just some comments to make our params more understandable

src/plugins/vis_type_xy/public/expression_functions/category_axis.ts

src/plugins/vis_type_xy/public/expression_functions/label.ts

src/plugins/vis_type_xy/public/expression_functions/category_axis.ts

src/plugins/vis_type_xy/public/expression_functions/series_param.ts

src/plugins/vis_type_xy/public/expression_functions/vis_scale.ts

src/plugins/vis_type_xy/public/xy_vis_fn.ts

stratoula · 2021-05-13T10:11:14Z

@nickofthyme do you want to also have a look? As you know this plugin better ❤️

stratoula · 2021-05-13T11:38:41Z

Another thing that concerns me is why the bundle size has been increased by +20.3KB. It seems quite big for this change. Can you look into it?

nickofthyme

This is a great refactor. I haven't run locally but all the code changes LGTM.

nickofthyme · 2021-05-14T12:46:58Z

src/plugins/vis_type_xy/public/to_ast.ts

  const visTypeXy = buildExpressionFunction<VisTypeXyExpressionFunctionDefinition>(visName, {
    type: vis.type.name as XyVisType,
-    visConfig: JSON.stringify(visConfig),
+    chartType: vis.params.type,
+    addTimeMarker: vis.params.addTimeMarker,
+    addLegend: vis.params.addLegend,
+    addTooltip: vis.params.addTooltip,
+    legendPosition: vis.params.legendPosition,
+    orderBucketsBySum: vis.params.orderBucketsBySum,
+    categoryAxes: vis.params.categoryAxes.map(prepareCategoryAxis),
+    valueAxes: vis.params.valueAxes.map(prepareValueAxis),
+    seriesParams: vis.params.seriesParams.map(prepareSeriesParam),
+    labels: prepareLabel(vis.params.labels),
+    thresholdLine: prepareThresholdLine(vis.params.thresholdLine),
+    gridCategoryLines: vis.params.grid.categoryLines,
+    gridValueAxis: vis.params.grid.valueAxis,
+    radiusRatio: vis.params.radiusRatio,
+    isVislibVis: vis.params.isVislibVis,
+    detailedTooltip: vis.params.detailedTooltip,
+    fittingFunction: vis.params.fittingFunction,
+    times: vis.params.times.map(prepareTimeMarker),
+    palette: vis.params.palette.name,
+    xDimension: dimensions.x ? prepareXYDimension(dimensions.x) : null,
+    yDimension: dimensions.y.map(prepareXYDimension),
+    zDimension: dimensions.z?.map(prepareXYDimension),
+    widthDimension: dimensions.width?.map(prepareXYDimension),
+    seriesDimension: dimensions.series?.map(prepareXYDimension),
+    splitRowDimension: dimensions.splitRow?.map(prepareXYDimension),
+    splitColumnDimension: dimensions.splitColumn?.map(prepareXYDimension),


This is great, love the flatter version.

nickofthyme · 2021-05-14T12:48:02Z

src/plugins/vis_type_xy/public/types/param.ts

@@ -116,7 +123,7 @@ export interface Dimensions {
  y: Dimension[];
  z?: Dimension[];
  width?: Dimension[];
-  series?: Dimension | Dimension[];
+  series?: Dimension[];


nickofthyme · 2021-05-14T12:51:41Z

src/plugins/vis_type_xy/public/to_ast.ts

@@ -62,15 +171,13 @@ export const toExpressionAst: VisToExpressionAst<VisParams> = async (vis, params
    }
  }

-  const visConfig = { ...vis.params };


This was to prevent an infinite update loop, particularly with date histogram charts. I think this should be fixed with these changes but could you play around with the date histogram visualization just updating the config and see if it hangs. Thanks!

I haven't find any problems with date histograms now. I guess you a re right and my PR fixed it.

VladLasitsa · 2021-05-15T13:10:36Z

@elasticmachine merge upstream

alexwizp · 2021-05-17T08:46:39Z

@elasticmachine merge upstream

alexwizp · 2021-05-17T12:30:34Z

src/plugins/vis_type_xy/public/types/constants.ts

@@ -6,52 +6,43 @@
 * Side Public License, v 1.
 */

-import { $Values } from '@kbn/utility-types';
-
-export const ChartMode = Object.freeze({


@nickofthyme it like an enum. What do you think about replacing this code with a clearer definition?

I think it's fine to change it back I just don't think babel does a great job transpiling them rn. See #53536 (comment)

alexwizp · 2021-05-17T14:17:38Z

@elasticmachine merge upstream

alexwizp

LGTM! Tested locally

stratoula

Thanx for the changes Vlad, a final small change on one description and it will be fine

src/plugins/vis_type_xy/public/expression_functions/label.ts

stratoula

LGTM, tested it locally and works fine

VladLasitsa · 2021-05-18T09:29:35Z

@elasticmachine merge upstream

kibanamachine · 2021-05-18T11:17:58Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visTypeXy`	91	99	+8

Async chunks

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

id	before	after	diff
`visTypeXy`	111.6KB	111.6KB	+20.0B

Page load bundle

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

id	before	after	diff
`visTypeXy`	43.4KB	62.7KB	+19.4KB

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`canvas`	29	25	-4
`crossClusterReplication`	8	6	-2
`fleet`	22	20	-2
`globalSearch`	4	2	-2
`indexManagement`	12	7	-5
`infra`	261	149	-112
`lens`	67	45	-22
`licensing`	18	15	-3
`maps`	286	208	-78
`ml`	121	115	-6
`monitoring`	109	56	-53
`stackAlerts`	101	95	-6
total			-295

History

💔 Build #126324 failed 2612cba
💚 Build #126270 succeeded 129f095
💔 Build #126237 failed 3d00b8e
💔 Build #126227 failed d6c6765
💔 Build #126212 failed 3dea787effad5e72b3158673bb90e8c88228829e

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

cc @VladLasitsa

timroes · 2021-05-18T12:16:31Z

@elasticmachine run elasticsearch-ci/docs

* Removed visconfig and using explicit params instead in xy_plugin * Fix CI * Fix i18n * Fix unit test * Fix some remarks * move expressions into separate chunk * fix CI * Update label.ts Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Alexey Antonov <[email protected]>

* Removed visconfig and using explicit params instead in xy_plugin * Fix CI * Fix i18n * Fix unit test * Fix some remarks * move expressions into separate chunk * fix CI * Update label.ts Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Alexey Antonov <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Alexey Antonov <[email protected]>

* Removed visconfig and using explicit params instead in xy_plugin * Fix CI * Fix i18n * Fix unit test * Fix some remarks * move expressions into separate chunk * fix CI * Update label.ts Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Alexey Antonov <[email protected]>

Removed visconfig and using explicit params instead in xy_plugin

3a3a068

VladLasitsa requested a review from alexwizp April 30, 2021 10:55

VladLasitsa added 3 commits April 30, 2021 16:37

Fix CI

419fa7c

Fix i18n

c56423c

Fix unit test

d81f574

Merge branch 'master' into 97247

9577eb6

Merge branch 'master' into 97247

8ff8add

VladLasitsa requested a review from stratoula May 12, 2021 07:55

VladLasitsa self-assigned this May 12, 2021

VladLasitsa added Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels May 12, 2021

VladLasitsa marked this pull request as ready for review May 12, 2021 08:00

VladLasitsa requested a review from a team May 12, 2021 08:00

stratoula added the Feature:XYAxis XY-Axis charts (bar, area, line) label May 13, 2021

Merge branch 'master' into 97247

8c88e04

stratoula reviewed May 13, 2021

View reviewed changes

stratoula requested a review from nickofthyme May 13, 2021 10:11

Fix some remarks

58e91a6

nickofthyme approved these changes May 14, 2021

View reviewed changes

Merge branch 'master' into 97247

5da5b24

Merge branch 'master' into 97247

71a2830

alexwizp reviewed May 17, 2021

View reviewed changes

move expressions into separate chunk

d6c6765

alexwizp force-pushed the 97247 branch from 3dea787 to d6c6765 Compare May 17, 2021 13:28

kibanamachine and others added 2 commits May 17, 2021 10:17

Merge branch 'master' into 97247

3d00b8e

fix CI

129f095

alexwizp force-pushed the 97247 branch from 1a164f0 to 129f095 Compare May 17, 2021 17:01

alexwizp approved these changes May 17, 2021

View reviewed changes

stratoula reviewed May 18, 2021

View reviewed changes

src/plugins/vis_type_xy/public/expression_functions/label.ts Outdated Show resolved Hide resolved

Update label.ts

2612cba

stratoula approved these changes May 18, 2021

View reviewed changes

Merge branch 'master' into 97247

84c8f5c

VladLasitsa merged commit 5c3527d into elastic:master May 18, 2021

VladLasitsa mentioned this pull request May 18, 2021

[7.x] [XY axis] Improve expression with explicit params (#98897) #100264

Merged

alexwizp mentioned this pull request Jun 23, 2022

[XY] Wrong visType for horizontal_bar visualization #135013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[XY axis] Improve expression with explicit params #98897

[XY axis] Improve expression with explicit params #98897

VladLasitsa commented Apr 30, 2021

VladLasitsa commented Apr 30, 2021

VladLasitsa commented May 12, 2021

elasticmachine commented May 12, 2021

stratoula commented May 13, 2021

stratoula left a comment

stratoula commented May 13, 2021

stratoula commented May 13, 2021

nickofthyme left a comment

nickofthyme May 14, 2021

nickofthyme May 14, 2021

nickofthyme May 14, 2021

VladLasitsa May 18, 2021

VladLasitsa commented May 15, 2021

alexwizp commented May 17, 2021

alexwizp May 17, 2021 •

edited

Loading

nickofthyme May 17, 2021

alexwizp commented May 17, 2021

alexwizp left a comment

stratoula left a comment

stratoula left a comment

VladLasitsa commented May 18, 2021

kibanamachine commented May 18, 2021

References to deprecated APIs

timroes commented May 18, 2021

[XY axis] Improve expression with explicit params #98897

[XY axis] Improve expression with explicit params #98897

Conversation

VladLasitsa commented Apr 30, 2021

Summary

VladLasitsa commented Apr 30, 2021

VladLasitsa commented May 12, 2021

elasticmachine commented May 12, 2021

stratoula commented May 13, 2021

stratoula left a comment

Choose a reason for hiding this comment

stratoula commented May 13, 2021

stratoula commented May 13, 2021

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme May 14, 2021

Choose a reason for hiding this comment

nickofthyme May 14, 2021

Choose a reason for hiding this comment

nickofthyme May 14, 2021

Choose a reason for hiding this comment

VladLasitsa May 18, 2021

Choose a reason for hiding this comment

VladLasitsa commented May 15, 2021

alexwizp commented May 17, 2021

alexwizp May 17, 2021 • edited Loading

Choose a reason for hiding this comment

nickofthyme May 17, 2021

Choose a reason for hiding this comment

alexwizp commented May 17, 2021

alexwizp left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

stratoula left a comment

Choose a reason for hiding this comment

VladLasitsa commented May 18, 2021

kibanamachine commented May 18, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

References to deprecated APIs

History

timroes commented May 18, 2021

alexwizp May 17, 2021 •

edited

Loading