[Canvas] HeatMap. by Kuznietsov · Pull Request #120239 · elastic/kibana

Kuznietsov · 2021-12-02T16:08:56Z

Summary

Heatmap chart has been integrated to Canvas.

Introduced changes:

Added default expressions to heatmap expression function.
Added color_picker with interactive palette to Canvas.
Added heatmap view, heatmap_grid and heatmap_legend models.
Added support of handling expression functions as arguments at Canvas sidebar panel.
Added support of adding models/views as arguments of the views/models at Canvas sidebar panel.
Added withDebounceArg HoC, which is making inputs more usable for users at Canvas sidebar panel.
Fixed number, string, range, percentage for supporting better UX without lagging and moving the cursor to the end of the line.
Reduced amount of arguments, passed to the Arg component, which affected the performance of the Canvas positively.

Screenshots

Support of expression functions as arguments:

Kuznietsov · 2021-12-07T09:13:31Z

@elasticmachine merge upstream

…ons.

Kuznietsov · 2021-12-15T07:04:30Z

@elasticmachine merge upstream

crob611

Looks good. Left a couple of comments that should be cleaned up and a couple of questions I'm curious about, but overall, I think it's good.

crob611 · 2021-12-22T17:08:24Z

x-pack/plugins/canvas/canvas_plugin_src/uis/arguments/color_picker/color_picker.tsx

@@ -0,0 +1,56 @@
+/*


Is there a reason we need this new color picker vs the existing color argument type? Color is probably not reusable outside of canvas, but it would at least keep consistency in Canvas that we can figure out once we make all of that available elsewhere?

I have had a couple of arguments in my head before implementing it:

ColorPicker at Canvas is totally our implementation. I believe, it has been created to hide the problems with reactDom.render on args update. Now we've got rid of such a problem.

Also, if I know correctly, Kibana is moving in the direction of relying on the eui components, to be able to change the UI very quickly. So, using the component, provided by the eui lib and not relying on the colors from the workpad directly in all the cases is a good idea, it seems to me.

@crob611, what do you think?

crob611 · 2021-12-22T17:17:08Z

x-pack/plugins/canvas/canvas_plugin_src/uis/models/heatmap_grid.ts

+ * 2.0.
+ */
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one


Remove double license header

crob611 · 2021-12-22T17:25:53Z

x-pack/plugins/canvas/public/components/arg_form/arg_form.js

          assets,
        };
-
+        // console.log(argumentProps);


Remove comment

crob611 · 2021-12-22T17:36:54Z

x-pack/plugins/canvas/public/components/arg_add_popover/arg_add_popover.tsx

-            key={`${opt.arg.name}-add`}
-            displayName={opt.arg.displayName ?? ''}
-            help={opt.arg.help ?? ''}
+            key={`${opt.name}-add`}


Question: What changed that led this to be the OptionArg?

The unnecessarily huge complexity of the configuration of the element, which just needs to display an array of labels)
The additional props, passed to the element, are leading to useless computations.

crob611 · 2021-12-22T17:57:27Z

x-pack/plugins/canvas/public/components/function_form/index.tsx

-          value: argValue,
-        })
-      );
+      dispatch(addArgumentValueAtIndex({ element, pageId, argName, value: argValue, path }));


Should probably change the name of this action since it no longer uses the index

crob611 · 2021-12-22T18:05:06Z

x-pack/plugins/canvas/public/components/with_debounce_arg/with_debounce_arg.tsx

+
+type Props = ArgTemplateFormProps['argumentProps'];
+
+export const withDebounceArg =


I really like this 👍

crob611 · 2021-12-22T18:23:12Z

x-pack/plugins/canvas/canvas_plugin_src/uis/arguments/datacolumn/index.js

 const DatacolumnArgInput = ({
  onValueChange,
-  columns,
+  resolved: { columns },


Can you explain the chain that led to some of these args having input come in via this resolved instead of directly as it previously was?

Sure. Before, at the Arg class, we were creating templateProps and passing there everything, but required only a few of the props, which were passed. So, I've reduced the number of props, we're passing and the performance has raised.

kibana/x-pack/plugins/canvas/public/expression_types/arg.ts

Line 142 in 1abcf81

const templateProps = {

But also, we have to control things, which are passed to the arguments.
At the previous version of this code you could see such logic:

const templateProps = { ...otherProps, ...resolved, onValueChange, typeInstance: this };

, where resolved are the props, which are produced by models/views/transforms resolve method. Example:

kibana/x-pack/plugins/canvas/canvas_plugin_src/uis/models/heatmap_grid.ts

Line 50 in 1abcf81

resolve({ context }: any): ResolvedColumns {

Such logic can cause some problems because it is impossible to track if some prop is rewritten. Also, it is difficult to document the types.

At this piece of code (

kibana/x-pack/plugins/canvas/public/expression_types/arg.ts

Line 148 in 1abcf81

resolved: { ...resolved, ...resolvedProps },

) you can see, that I've localized the object, produced by resolve method and now it is easier to track and understand the way, how our code is really working, and which props are default, and which are specific for our view/model/etc.

…hanged comments.

Kuznietsov · 2021-12-23T15:37:14Z

@elasticmachine merge upstream

Kuznietsov · 2021-12-24T08:44:18Z

@stratoula, when you'll review the current PR again, please, tell me if you agree with these changes: d0383a6. Thanks )

Kuznietsov · 2021-12-28T11:53:12Z

@elasticmachine merge upstream

stratoula · 2022-01-04T09:25:23Z

@elasticmachine merge upstream

stratoula · 2022-01-04T09:39:21Z

@Kunzetsov I think that the only required dimension should be the valueAccessor.
In Lens the vertical axis is not required

and in vizualize editor only the value accessor is required

With that being said I think is better to not apply these changes

Kuznietsov · 2022-01-04T09:54:26Z

@Kunzetsov I think that the only required dimension should be the valueAccessor. In Lens the vertical axis is not required

and in vizualize editor only the value accessor is required

With that being said I think is better to not apply these changes

@stratoula, I've played with these options before and they are causing troubles with required arguments. So, I've left it as it was before.

kibana/x-pack/plugins/canvas/canvas_plugin_src/uis/views/heatmap.ts

Line 35 in 687b83b

name: 'valueAccessor',

stratoula

Vis editor changes LGTM! I haven' checked a lot the canvas addition but I made some tests on viz editor and lens heatmaps. They work as expected.

I just want from @dej611 to have a look too just to have another pair of 👁️

dej611

Checked vis-editors changes and LGTM 👍

Kuznietsov · 2022-01-04T13:46:29Z

@stratoula, @dej611, @crob611, thanks for your reviews )

Kuznietsov · 2022-01-05T09:50:32Z

@elasticmachine merge upstream

Kuznietsov · 2022-01-06T08:15:22Z

@elasticmachine merge upstream

Kuznietsov · 2022-01-10T07:58:38Z

@elasticmachine merge upstream

kibana-ci · 2022-01-10T09:17:50Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`canvas`	1069	1078	+9

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
`expressionHeatmap`	111	95	-16

Async chunks

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

id	before	after	diff
`canvas`	1011.1KB	1.0MB	+16.5KB
`expressionHeatmap`	14.9KB	15.0KB	+132.0B
`lens`	1021.1KB	1020.8KB	-283.0B
total			+16.3KB

Page load bundle

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

id	before	after	diff
`expressionHeatmap`	12.9KB	12.2KB	-656.0B

Unknown metric groups

API count

id	before	after	diff
`expressionHeatmap`	115	99	-16

History

💚 Build #16073 succeeded e245aba
💚 Build #15852 succeeded ebab20c
💚 Build #15595 succeeded 687b83b
💚 Build #15224 succeeded 0dc0d2a
💚 Build #15065 succeeded 465d6b7
💔 Build #15051 failed d0383a6

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

cc @Kunzetsov

crob611

Looks great. Excited to have this in Canvas

Kuznietsov added 2 commits December 2, 2021 17:08

small types refactoring

2477e95

Added defaults to expressions,.

267bec0

Kuznietsov requested review from alexwizp, crob611 and stratoula December 2, 2021 16:08

Kuznietsov self-assigned this Dec 2, 2021

Kuznietsov added the WIP Work in progress label Dec 2, 2021

Kuznietsov requested a review from flash1293 December 3, 2021 07:58

kibanamachine and others added 4 commits December 7, 2021 04:13

Merge branch 'main' into canvas-heat-map

6ef75d7

Fixed error on number passed as a name.

a47d27c

One more fix.

b3c363e

Added Heatmap element to Canvas.

d6d9991

Kuznietsov linked an issue Dec 8, 2021 that may be closed by this pull request

[Heatmap] Not used expression functions arguments. #120632

Closed

Kuznietsov added 9 commits December 10, 2021 13:45

Added support of nested expressions.

b0c4375

Added support of the adding models/views as arguments of the expressi…

80ec4ab

…ons.

Added support of the name from parent configuration.

782780f

Added support of removing nested models.

32e1573

Added heatmap legend description)

a630c5d

Fixed removing logic.

dae1a64

Small refactoring

5bb8616

Replaced help and displayName of legend.

dd6ac95

Added heatmap_grid.

b05abc0

kibanamachine and others added 6 commits December 15, 2021 02:04

Merge branch 'main' into canvas-heat-map

869789d

Fixed label.

09410fb

Added context of nested expressions support.

db7e948

Small refactoring.

7cefb92

Fixed bugs with updating of elements.

7b9d58b

Added color picker.

f8b7aa6

stratoula requested a review from dej611 December 22, 2021 12:55

crob611 approved these changes Dec 22, 2021

View reviewed changes

Kuznietsov added 2 commits December 23, 2021 09:44

Removed forgotten comments.

3149f9c

Changed names of setArgumentAtIndex and addArgumentValueAtIndex and c…

1abcf81

…hanged comments.

kibanamachine and others added 2 commits December 23, 2021 10:37

Merge branch 'main' into canvas-heat-map

13937a1

Added "required" fields.

d0383a6

Removed "required", which are breaking vis_editors.

465d6b7

Merge branch 'main' into canvas-heat-map

0dc0d2a

Merge branch 'main' into canvas-heat-map

687b83b

stratoula approved these changes Jan 4, 2022

View reviewed changes

dej611 approved these changes Jan 4, 2022

View reviewed changes

Merge branch 'main' into canvas-heat-map

ebab20c

Merge branch 'main' into canvas-heat-map

e245aba

Merge branch 'main' into canvas-heat-map

4c1e505

crob611 approved these changes Jan 11, 2022

View reviewed changes

Kuznietsov merged commit dd9d846 into elastic:main Jan 12, 2022

Kuznietsov removed the WIP Work in progress label Jan 12, 2022

dej611 mentioned this pull request Jan 17, 2022

[Lens] Duplicated datatable available as inspector data for Heatmap chart #123176

Closed

alexwizp mentioned this pull request Mar 14, 2022

[Lens] Duplicated datatable available as inspector data for Heatmap chart #126786

Merged


		type Props = ArgTemplateFormProps['argumentProps'];

		export const withDebounceArg =

Conversation

Kuznietsov commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Support of expression functions as arguments:

Uh oh!

Kuznietsov commented Dec 7, 2021

Uh oh!

Kuznietsov commented Dec 15, 2021

Uh oh!

crob611 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kuznietsov Dec 23, 2021 • 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 Dec 23, 2021

Uh oh!

Kuznietsov commented Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kuznietsov commented Dec 28, 2021

Uh oh!

stratoula commented Jan 4, 2022

Uh oh!

stratoula commented Jan 4, 2022

Uh oh!

Kuznietsov commented Jan 4, 2022

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

dej611 left a comment

Choose a reason for hiding this comment

Uh oh!

Kuznietsov commented Jan 4, 2022

Uh oh!

Kuznietsov commented Jan 5, 2022

Uh oh!

Kuznietsov commented Jan 6, 2022

Uh oh!

Kuznietsov commented Jan 10, 2022

Uh oh!

kibana-ci commented Jan 10, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

crob611 left a comment

Kuznietsov commented Dec 2, 2021 •

edited

Loading

Kuznietsov Dec 23, 2021 •

edited

Loading

Kuznietsov commented Dec 24, 2021 •

edited

Loading