[Lens] update default legend size to medium #130336

drewdaemon · 2022-04-14T21:18:43Z

Summary

Defaults legend size to medium instead of "auto" and gently removes the user's ability to select the "auto" option.

Also migrates visualizations to use string-based instead of numerical values for legend sizes (e.g. 'small' instead of 80). This will make the saved objects more readable and make the code more maintainable as well as increasing our flexibility to change the definitions of the legend sizes in the future in a uniform way.

Testing

"Medium" legend size for new visualizations

Create some of the following types of visualizations and assert that the legend size is medium for each of them.

Lens: pie/mosaic/(any other partiion type), heatmap, XY
Visualize: pie, area, histogram, horizontal_bar, line, heatmap

"Auto" preserved for existing visualizations

Import these dashboards and for each visualization:

assert that it retains "Auto" as its legend size
change legend size to something else
make sure "Auto" is still an option in the list
save visualization
navigate away from editor
navigate back to editor
make sure "Auto" has disappeared from the list

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

…zations

…-fix'

…' of github.com:andrewctate/kibana into 129474/default-legend-size-to-fixed-migrations-strategy

drewdaemon · 2022-04-19T16:22:44Z

src/plugins/vis_default_editor/kibana.json

+    "fieldFormats",
+    "discover",
+    "esUiShared",
+    "visualizations"


Adding visualizations as a required bundle is the salient change here

drewdaemon · 2022-04-19T16:24:23Z

src/plugins/visualizations/common/constants.ts

+export const LegendSizes = {
+  AUTO: 0,
+  SMALL: 80,
+  MEDIUM: 130,
+  LARGE: 180,
+  EXTRA_LARGE: 230,
+} as const;
+
+export const DEFAULT_LEGEND_SIZE = LegendSizes.MEDIUM;


This seemed like the right place to put these for usage in all the vis_types. Still open to input.

If we have the legend sizes here, do we still need them in the Lens plugin? https://github.com/elastic/kibana/pull/130336/files#diff-d23e749e762688055d5cdbc52ed902a26a009743d66f901b1568728975bcbb9cR45

drewdaemon · 2022-04-19T18:15:26Z

@elasticmachine merge upstream

…ns-strategy

…' of github.com:andrewctate/kibana into 129474/default-legend-size-to-fixed-migrations-strategy

drewdaemon · 2022-04-19T21:19:24Z

@MichaelMarcialis as mentioned here in the original issue, we want to remove the ability to select "Auto" as a legend size. However, we are keeping existing visualizations that are using "Auto" the same until the owner has updated the legend size.

I currently have this set up so that when the user of a pre-existing visualization selects a legend size other than "Auto," the "Auto" option disappears from the dropdown. @timductive pointed out that this could be annoying or confusing.

Do you have any input on how we can safely go about turning off the "Auto" option once the user has selected a different size?

Screen.Recording.2022-04-19.at.4.18.47.PM.mov

drewdaemon · 2022-04-20T13:45:14Z

@elasticmachine merge upstream

…ns-strategy

MichaelMarcialis · 2022-04-20T14:47:44Z

@MichaelMarcialis as mentioned here in the original issue, we want to remove the ability to select "Auto" as a legend size. However, we are keeping existing visualizations that are using "Auto" the same until the owner has updated the legend size.

I currently have this set up so that when the user of a pre-existing visualization selects a legend size other than "Auto," the "Auto" option disappears from the dropdown. @timductive pointed out that this could be annoying or confusing.

Do you have any input on how we can safely go about turning off the "Auto" option once the user has selected a different size?

@andrewctate: Perhaps we could delay the removal of the "Auto" option in these situations until after the user has 1) changed to a size other than "Auto" and 2) has saved their visualization. Once saved (likely at an ideal state for the user), then we can feel more comfortable in removing that auto option. Thoughts?

On a side note, would it be possible to add flanking prepend/append icon buttons to the selector to decrease/increase the currently chosen size respectively? I didn't recommend it initially, due to the inclusion of the "Auto" option, but once that option is removed, I think we can safely add in such shortcut controls. We take a similar approach in the metric visualization text size options, and though it would be a good pattern here as well. Just let me know if you'd prefer that spun off into a separate issue instead.

drewdaemon · 2022-04-20T17:53:42Z

@MichaelMarcialis that plan sounds good to me.

Let's take the updated UI controls as a separate issue in the name of smaller, more focused changesets.

…' of github.com:andrewctate/kibana into 129474/default-legend-size-to-fixed-migrations-strategy

flash1293 · 2022-05-04T13:36:41Z

It seems like the migration doesn't work correctly for xy charts (both lens and agg based). I created a dashboard with by value and by reference panels in 8.3 and the legend size looks like this:

Not how it seems to work fine for pies (the last row are treemap visualizations)

drewdaemon · 2022-05-04T16:24:43Z

@flash1293 that display issue should be resolved now

drewdaemon · 2022-05-04T16:30:21Z

src/plugins/visualizations/common/constants.ts

@@ -26,3 +26,13 @@ export const VisualizeConstants = {
  EDIT_BY_VALUE_PATH: '/edit_by_value',
  APP_ID: 'visualize',
 };
+
+export const LegendSizes = {
+  AUTO: 0,


In the spirit of future-proofing, can we think of any case where someone would actually want a legend size of zero? We could use a different number like -1 to denote "auto" if we think zero should be left available.

cc: @flash1293

flash1293 · 2022-05-05T08:33:54Z

Tested and this works fine now. About

In the spirit of future-proofing, can we think of any case where someone would actually want a legend size of zero? We could use a different number like -1 to denote "auto" if we think zero should be left available.

@markov00 made a good point about this offline - what about storing an enum in the saved object instead (auto/small/medium/large) and only mapping that to a pixel number in the toExpression function? It would make problems like 0 vs auto I encountered above much harder to miss and give us additional flexibility later on (allowing pixel value OR enum string).

…legend-size-to-fixed-migrations-strategy

drewdaemon · 2022-05-05T16:06:18Z

@markov00 made a good point about this offline - what about storing an enum in the saved object instead (auto/small/medium/large) and only mapping that to a pixel number in the toExpression function? It would make problems like 0 vs auto I encountered above much harder to miss and give us additional flexibility later on (allowing pixel value OR enum string).

This was a great idea. I was experimenting with auto as a special string case with the other sizes staying numbers, but it got messy. Making them all strings is much more consistent and has the other benefits ^^ as well!

drewdaemon · 2022-05-05T18:14:24Z

@flash1293 @Kunzetsov I'm starting to wonder if we should push the legend-size-to-pixel translation one level deeper than each visualization's toExpression function. Do you think it makes sense to have the chart expression functions accept the legend size strings (auto, small, medium, etc) instead of a pixel number?

Seems like there's precedent for this with the legendDisplay and legendPosition arguments.

    legendDisplay: {
      types: ['string'],
      help: strings.getLegendDisplayArgHelp(),
      options: [LegendDisplay.SHOW, LegendDisplay.HIDE, LegendDisplay.DEFAULT],
      default: LegendDisplay.HIDE,
      strict: true,
    },
    legendPosition: {
      types: ['string'],
      default: Position.Right,
      help: strings.getLegendPositionArgHelp(),
      options: [Position.Top, Position.Right, Position.Bottom, Position.Left],
      strict: true,
    },

An added benefit is that we wouldn't have to split the current visualization config interfaces (example) into two, one for the saved objects and one for the expression arguments.

flash1293 · 2022-05-05T18:40:20Z

Sounds good to me - it makes sense to keep the abstraction layers aligned if possible.

…legend-size-to-fixed-migrations-strategy

src/plugins/vis_types/pie/public/to_ast.ts

drewdaemon · 2022-05-09T13:23:34Z

@elasticmachine merge upstream

…ns-strategy

flash1293 · 2022-05-10T09:27:02Z

@elasticmachine merge upstream

…ns-strategy

flash1293

Tested and works as expected, LGTM

kibana-ci · 2022-05-10T10:24:49Z

💚 Build Succeeded

Metrics [docs]

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`	100	103	+3
`visualizations`	344	350	+6
total			+9

Async chunks

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

id	before	after	diff
`expressionHeatmap`	80.7KB	80.8KB	+91.0B
`expressionPartitionVis`	44.1KB	44.2KB	+83.0B
`expressionXY`	55.8KB	55.9KB	+86.0B
`lens`	1.1MB	1.1MB	+326.0B
`visDefaultEditor`	142.4KB	142.4KB	+1.0B
`visTypeHeatmap`	7.4KB	7.5KB	+80.0B
`visTypePie`	8.9KB	9.0KB	+80.0B
`visTypeXy`	60.3KB	60.5KB	+165.0B
total			+912.0B

Page load bundle

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

id	before	after	diff
`expressionHeatmap`	14.0KB	14.3KB	+304.0B
`expressionPartitionVis`	21.9KB	22.7KB	+756.0B
`expressionXY`	27.5KB	27.8KB	+303.0B
`lens`	32.5KB	32.6KB	+154.0B
`visDefaultEditor`	21.1KB	21.3KB	+124.0B
`visTypePie`	8.1KB	8.0KB	-102.0B
`visTypeXy`	43.2KB	43.3KB	+141.0B
`visualizations`	45.9KB	46.5KB	+559.0B
total			+2.2KB

Unknown metric groups

API count

id	before	after	diff
`expressionHeatmap`	104	107	+3
`visualizations`	365	371	+6
total			+9

History

💚 Build #43329 succeeded bc69ec8
💚 Build #43218 succeeded cee8d53
💔 Build #43187 failed cd85c83
💔 Build #43148 failed 701a66c
💔 Build #43066 failed 58b4dcd
💔 Build #43055 failed 8c28bbc

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

drewdaemon added 7 commits April 13, 2022 12:55

update lens legend default to medium

1bd3e8a

Remove auto option when not currently selected

348b793

update legend size settings

a6449c6

Add lens legend migration

43011c6

Default legend size at expression level

a43e87a

hide auto option and update default legend size for agg-based visuali…

f50405c

…zations

update default legend sizes for aggs-based visualizations

bacdef6

drewdaemon force-pushed the 129474/default-legend-size-to-fixed-migrations-strategy branch from 10cc455 to bacdef6 Compare April 15, 2022 16:32

drewdaemon and others added 4 commits April 15, 2022 12:56

fn name update

07b6ba4

add migration for aggs-based visualizations

75e5770

Add visualizations as required bundle for editor

cbd7483

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

f7079d2

…-fix'

drewdaemon changed the title ~~[Lens] update default legend size to medium - yes-migration strategy~~ [Lens] update default legend size to medium Apr 19, 2022

drewdaemon added 2 commits April 19, 2022 11:16

add test for visualize SO migration

9e97779

Merge branch '129474/default-legend-size-to-fixed-migrations-strategy…

6e87e68

…' of github.com:andrewctate/kibana into 129474/default-legend-size-to-fixed-migrations-strategy

drewdaemon commented Apr 19, 2022

View reviewed changes

drewdaemon added 2 commits April 19, 2022 13:10

test lens saved object migrations

e252436

update unit test

c018293

kibanamachine and others added 3 commits April 19, 2022 13:15

Merge branch 'main' into 129474/default-legend-size-to-fixed-migratio…

ce369ec

…ns-strategy

update baseline screenshot for area chart

1be5bbf

Merge branch '129474/default-legend-size-to-fixed-migrations-strategy…

538742a

…' of github.com:andrewctate/kibana into 129474/default-legend-size-to-fixed-migrations-strategy

Merge branch 'main' into 129474/default-legend-size-to-fixed-migratio…

7f879f1

…ns-strategy

drewdaemon added 2 commits April 21, 2022 12:53

wait to hide auto option - lens

0efec68

Merge branch '129474/default-legend-size-to-fixed-migrations-strategy…

17140fc

…' of github.com:andrewctate/kibana into 129474/default-legend-size-to-fixed-migrations-strategy

Interpret auto as auto instead of 0

0e3a30d

drewdaemon commented May 4, 2022

View reviewed changes

drewdaemon requested a review from flash1293 May 4, 2022 16:30

update baseline snapshot

922b5a9

drewdaemon added 2 commits May 5, 2022 11:04

Using strings for legend sizes

3a3cf3a

Merge branch 'main' of github.com:elastic/kibana into 129474/default-…

43a69e9

…legend-size-to-fixed-migrations-strategy

drewdaemon added 9 commits May 5, 2022 16:39

move legendSize default and pixel translation to expression layer

bf828cb

update aggs-based

8c28bbc

update some unit tests

c4f118d

remove unused import

58b4dcd

Merge branch 'main' of github.com:elastic/kibana into 129474/default-…

eb4588f

…legend-size-to-fixed-migrations-strategy

remove unused imports

3332ce1

update snaps

701a66c

update translation path

cd85c83

restore i18n string

cee8d53

markov00 reviewed May 9, 2022

View reviewed changes

src/plugins/vis_types/pie/public/to_ast.ts Outdated Show resolved Hide resolved

remove unnecessary optional chaining operators

9ee0704

Merge branch 'main' into 129474/default-legend-size-to-fixed-migratio…

bc69ec8

…ns-strategy

Merge branch 'main' into 129474/default-legend-size-to-fixed-migratio…

d612e4d

…ns-strategy

flash1293 approved these changes May 10, 2022

View reviewed changes

drewdaemon merged commit 57597f7 into elastic:main May 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] update default legend size to medium #130336

[Lens] update default legend size to medium #130336

drewdaemon commented Apr 14, 2022 •

edited

Loading

drewdaemon Apr 19, 2022

drewdaemon Apr 19, 2022

flash1293 May 3, 2022

drewdaemon commented Apr 19, 2022

drewdaemon commented Apr 19, 2022

drewdaemon commented Apr 20, 2022

MichaelMarcialis commented Apr 20, 2022

drewdaemon commented Apr 20, 2022

flash1293 commented May 4, 2022 •

edited

Loading

drewdaemon commented May 4, 2022

drewdaemon May 4, 2022

flash1293 commented May 5, 2022

drewdaemon commented May 5, 2022

drewdaemon commented May 5, 2022

flash1293 commented May 5, 2022

drewdaemon commented May 9, 2022

flash1293 commented May 10, 2022

flash1293 left a comment

kibana-ci commented May 10, 2022

API count

[Lens] update default legend size to medium #130336

[Lens] update default legend size to medium #130336

Conversation

drewdaemon commented Apr 14, 2022 • edited Loading

Summary

Testing

"Medium" legend size for new visualizations

"Auto" preserved for existing visualizations

Checklist

drewdaemon Apr 19, 2022

Choose a reason for hiding this comment

drewdaemon Apr 19, 2022

Choose a reason for hiding this comment

flash1293 May 3, 2022

Choose a reason for hiding this comment

drewdaemon commented Apr 19, 2022

drewdaemon commented Apr 19, 2022

drewdaemon commented Apr 20, 2022

MichaelMarcialis commented Apr 20, 2022

drewdaemon commented Apr 20, 2022

flash1293 commented May 4, 2022 • edited Loading

drewdaemon commented May 4, 2022

drewdaemon May 4, 2022

Choose a reason for hiding this comment

flash1293 commented May 5, 2022

drewdaemon commented May 5, 2022

drewdaemon commented May 5, 2022

flash1293 commented May 5, 2022

drewdaemon commented May 9, 2022

flash1293 commented May 10, 2022

flash1293 left a comment

Choose a reason for hiding this comment

kibana-ci commented May 10, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

drewdaemon commented Apr 14, 2022 •

edited

Loading

flash1293 commented May 4, 2022 •

edited

Loading