[Lens] Pie and donuts should have a size ratio setting by DianaDerevyankina · Pull Request #120101 · elastic/kibana

DianaDerevyankina · 2021-12-01T14:06:22Z

Summary

Added a setting for classic Pie and Lens Donut visualizations that allow to configure the donut thickness. Previous ratio is now "Small" and selected by default.

setting is hidden for visualization:visualize:legacyPieChartsLibrary

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

For maintainers

This was checked for breaking API changes and was labeled appropriately

Closes: elastic#107059

# Conflicts: # x-pack/plugins/lens/public/pie_visualization/constants.ts # x-pack/plugins/lens/public/pie_visualization/partition_charts_meta.ts # x-pack/plugins/lens/public/pie_visualization/to_expression.ts # x-pack/plugins/lens/public/pie_visualization/toolbar.tsx

…for lens

alexwizp · 2021-12-06T14:35:04Z

src/plugins/vis_types/pie/public/editor/components/pie.tsx

+                  ({ value }) => value === stateParams.donutInnerAreaSize
+                )?.id || 'donutInnerAreaSizeOption-medium'
+              }
+              onChange={(sizeId) => {


plz move into useCallback

alexwizp · 2021-12-06T14:35:37Z

src/plugins/vis_types/pie/public/editor/components/pie.tsx

+              idSelected={
+                donutInnerAreaSizeOptions.find(
+                  ({ value }) => value === stateParams.donutInnerAreaSize
+                )?.id || 'donutInnerAreaSizeOption-medium'


nit: with ? we normally use ?? instead of ||

alexwizp · 2021-12-06T14:43:57Z

x-pack/plugins/lens/public/pie_visualization/partition_charts_meta.ts

      value: SharedPieLayerState['numberDisplay'];
      inputDisplay: string;
    }>;
+    donutInnerAreaSizeOptions: Array<{


metadata should keep a generic data. let's exclude everything with a special chart name

alexwizp · 2021-12-06T20:38:00Z

x-pack/plugins/lens/public/pie_visualization/partition_charts_meta.ts

      value: SharedPieLayerState['numberDisplay'];
      inputDisplay: string;
    }>;
+    innerAreaSizeOptions: Array<{


can we make it optional?

alexwizp · 2021-12-06T20:40:02Z

x-pack/plugins/lens/public/pie_visualization/render_function.tsx

    legendPosition,
    nestedLegend,
    percentDecimals,
+    donutInnerAreaSize,


I don't understand why you use donutInnerAreaSize name instead of generic emptySizeRatio?

elasticmachine · 2021-12-07T13:29:39Z

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

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

stratoula

@dziyanadzeraviankina this looks great. Let's add some unit tests as the options appear under specific condtions.

@ghudgins @MichaelMarcialis can you also take a look and let us know if the selected ratios seem fine?

stratoula · 2021-12-09T07:14:14Z

src/plugins/vis_types/pie/public/editor/components/pie.tsx

          value={stateParams.isDonut}
          setValue={setValue}
        />
+        {props.showElasticChartsOptions && stateParams.isDonut && (


Can you cover this functionality in the pie.test? I have other cases there, let's test that it is hidden when vislib is on or if it is a pie.
Also can we also add a test for lens to test that the visual options menu is only visible when we have a donut partition chart and not a pie or treemap etc

MichaelMarcialis · 2021-12-13T20:52:58Z

@ghudgins @MichaelMarcialis can you also take a look and let us know if the selected ratios seem fine?

To my eyes, the ratio for the size differences between small-to-medium and medium-to-large appear to be quite large. For example, moving from small-top-medium appears as a small increase in the donut hole size, whereas going from medium-to-large appears as a very large increase in donut hole size. Would it be possible to distribute these sizes more evenly? Or is there a specific reason for this? CCing @gvnmagni in case he has additional thoughts.

Also, as a small side thought, would Donut hole size be a better label for this field, instead of Inner area size?

ghudgins · 2021-12-13T21:00:26Z

agreed at the options feeling a bit off. The large looks good but small & medium look both pretty small. I think the reasoning for this was to provide more space for the interior label but it's hard to imagine anyone who edits this setting changing to small (from medium) since they're so similar.

stratoula · 2021-12-14T08:28:55Z

@MichaelMarcialis @ghudgins I agree with your point. If we give only two options? (Large which will give the same ratio as vislib pie) and Normal (which will be what we have so far?) We can change the naming of course but maybe the small hole doesn't make any difference.

gvnmagni · 2021-12-14T08:43:38Z

Thank you @MichaelMarcialis for tagging me! I'm not aware of any specific rule about this ratio (a part avoiding holes too big or too small), I'd say that we should follow our common sense. I'd distribute these 3 ratio in order to have the distance between them consistent

@monfera any thought about this? Any rule/practice that I'm not aware of?

flash1293 · 2021-12-14T08:46:36Z

@gvnmagni

I'd distribute these 3 ratio in order to have the distance between them consistent

Consistent in terms of radius or area of the hole? I think the later would make more sense here.

alexwizp

code lgtm

gvnmagni · 2021-12-14T09:25:57Z

@gvnmagni

I'd distribute these 3 ratio in order to have the distance between them consistent

Consistent in terms of radius or area of the hole? I think the later would make more sense here.

Agree! Even if in this case we are not encoding data with the area I'd say that we should always consider the area when dealing with these kind of chart. Thank you for mentioning this, I gave it for granted :)

flash1293 · 2021-12-16T16:07:47Z

@elasticmachine merge upstream

flash1293

So I don't have a strong opinion, but what about 0.3, 0.54 and 0.7 (defaulting to 0.3)?

This will make the change in area consistent giving the user options without changing our current default donut.

0.3 has an area of 0.28274333882
0.54 has an area of 0.91608841778
0.7 has an area of 1.53938040026

Otherwise this PR looks good to me (agree with Stratoulas comments), we can also merge it without a final answer to the question on the hole ratios as it's easy to change afterwards

flash1293

LGTM

DianaDerevyankina · 2021-12-20T08:11:38Z

@elasticmachine merge upstream

kibana-ci · 2021-12-20T09:30:18Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`visTypePie`	47	48	+1

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
`lens`	245	246	+1

Async chunks

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

id	before	after	diff
`lens`	1018.9KB	1020.4KB	+1.5KB
`visTypePie`	50.4KB	51.5KB	+1.1KB
total			+2.6KB

Page load bundle

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

id	before	after	diff
`lens`	40.9KB	41.0KB	+42.0B
`visTypePie`	14.0KB	14.4KB	+407.0B
total			+449.0B

Unknown metric groups

API count

id	before	after	diff
`lens`	262	263	+1

History

💚 Build #13945 succeeded af1899d
💚 Build #13686 succeeded d67036d
💚 Build #12396 succeeded a9cb5ab
💚 Build #12086 succeeded 0bdd7da
💚 Build #12055 succeeded 65e6f66
💚 Build #11731 succeeded 3c62ded

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

cc @dziyanadzeraviankina

* [WIP][Lens] Waffle visualization type Closes: elastic#107059 * add showExtraLegend for waffle * add tests * resolved 1 and 5 * resolved 6 * add sortPredicate for waffle chart type * [Lens] Pie and donuts should have a size ratio setting * Add a missed condition * Fix changing size for smallSlices * Add donut inner area size setting to pie visualization and update it for lens * Update test and rename some constants * Rename the setting * Move handler to a separate useCallback function * Update size ratios and add condition for legacy charts * Fix merge conflict issue * Change constants order * Add a couple of tests to check if the setting is displayed * Update ratio sizes Co-authored-by: Alexey Antonov <alexwizp@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

alexwizp and others added 12 commits November 23, 2021 17:36

[WIP][Lens] Waffle visualization type

a5c67a7

Closes: elastic#107059

add showExtraLegend for waffle

3a58f88

add tests

7291eef

Merge branch 'main' into 107059

2824beb

Merge branch 'main' into 107059

1d8e18b

resolved 1 and 5

34f6cb6

resolved 6

33ee2be

Merge branch 'main' into 107059

6828779

Merge branch 'main' into 107059

8898eed

add sortPredicate for waffle chart type

76ff6e9

[Lens] Pie and donuts should have a size ratio setting

137a82d

Add a missed condition

7a40ed1

DianaDerevyankina self-assigned this Dec 1, 2021

DianaDerevyankina added Feature:Lens release_note:enhancement Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// backport:skip This PR does not require backporting v8.1.0 labels Dec 2, 2021

Fix changing size for smallSlices

f7907ad

DianaDerevyankina requested a review from alexwizp December 2, 2021 11:32

Add donut inner area size setting to pie visualization and update it …

4833ef5

…for lens

alexwizp reviewed Dec 6, 2021

View reviewed changes

Update test and rename some constants

d8eb0b5

alexwizp reviewed Dec 6, 2021

View reviewed changes

DianaDerevyankina added 2 commits December 7, 2021 11:51

Rename the setting

dd5c11c

Move handler to a separate useCallback function

dae7ada

Update size ratios and add condition for legacy charts

3c62ded

DianaDerevyankina marked this pull request as ready for review December 7, 2021 13:29

DianaDerevyankina requested a review from a team as a code owner December 7, 2021 13:29

DianaDerevyankina added 3 commits December 8, 2021 12:14

Merge branch 'main' into issues/65538

d87315a

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

Fix merge conflict issue

65e6f66

Change constants order

0bdd7da

stratoula reviewed Dec 9, 2021

View reviewed changes

DianaDerevyankina added 2 commits December 9, 2021 13:54

Merge branch 'main' into issues/65538

b0f62d9

Add a couple of tests to check if the setting is displayed

a9cb5ab

alexwizp approved these changes Dec 14, 2021

View reviewed changes

Merge branch 'main' into issues/65538

d67036d

flash1293 reviewed Dec 16, 2021

View reviewed changes

Update ratio sizes

af1899d

flash1293 approved these changes Dec 17, 2021

View reviewed changes

Merge branch 'main' into issues/65538

fdda604

DianaDerevyankina merged commit 2d2d702 into elastic:main Dec 20, 2021

Conversation

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

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

alexwizp Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

alexwizp Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

alexwizp Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

alexwizp Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

alexwizp Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 7, 2021

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

stratoula Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

MichaelMarcialis commented Dec 13, 2021

Uh oh!

ghudgins commented Dec 13, 2021

Uh oh!

stratoula commented Dec 14, 2021

Uh oh!

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

Uh oh!

Uh oh!

flash1293 commented Dec 14, 2021

Uh oh!

alexwizp 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!

gvnmagni commented Dec 14, 2021

Uh oh!

flash1293 commented Dec 16, 2021

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

DianaDerevyankina commented Dec 20, 2021

Uh oh!

kibana-ci commented Dec 20, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DianaDerevyankina commented Dec 1, 2021 •

edited

Loading

gvnmagni commented Dec 14, 2021 •

edited

Loading

alexwizp left a comment •

edited

Loading