Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Lens] Waffle - add "show values in legend" configuration property #120471

Merged
merged 4 commits into from
Dec 7, 2021
Merged

[Lens] Waffle - add "show values in legend" configuration property #120471

merged 4 commits into from
Dec 7, 2021

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Dec 6, 2021 •
edited
Loading

Summary

[Lens] Waffle - add "show values in legend" configuration property

This PR address the following comment #119339 (review):

A suggestion as a follow up PR would be to have a way to not display values on legend, but just labels. Sometimes it's ok to have just a rough idea of quantities without knowing the exact amount.

Screenshot 2021-12-01 at 17 36 50

Screens:
image

Show Value configuration property was added

@alexwizp alexwizp changed the title [Lens] Mosaic - add "show values in legend" configuration property for Waffle chart type [Lens] Waffle - add "show values in legend" configuration property Dec 6, 2021
alexwizp
alexwizp commented Dec 6, 2021
View reviewed changes
x-pack/plugins/lens/public/pie_visualization/toolbar.tsx Outdated
}}
onDisplayChange={onLegendDisplayChange}
valueInLegend={shouldShowValuesInLegend(layer, state.shape)}
renderValueInLegendSwitch={Boolean(PartitionChartsMeta[state.shape]?.legend?.showValues)}
Copy link
Contributor Author

@alexwizp alexwizp Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flash1293 @ghudgins added "Show Value" for Waffle only. What are your thoughts on adding this property for other partition charts?

Copy link
Contributor

@ghudgins ghudgins Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any downsides to allowing this. on the mosaic specifically i'm not sure it makes sense to display overall metrics in the legends given you aren't visualizing that metric but maybe it's a nice piece of extra information when you aren't hovering?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for just allowing this for waffle for now.

@alexwizp alexwizp self-assigned this Dec 6, 2021
@alexwizp alexwizp added v8.1.0 Feature:Lens backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Dec 6, 2021
@alexwizp alexwizp marked this pull request as ready for review December 6, 2021 14:15
@alexwizp alexwizp requested a review from a team as a code owner December 6, 2021 14:15
@elasticmachine
Copy link
Contributor

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

@alexwizp alexwizp requested a review from VladLasitsa December 6, 2021 14:33
flash1293
flash1293 reviewed Dec 6, 2021
View reviewed changes
x-pack/plugins/lens/public/pie_visualization/render_helpers.ts Outdated
@@ -44,6 +45,11 @@ export const isPartitionShape = (shape: PieChartTypes | string) =>
export const isTreemapOrMosaicShape = (shape: PieChartTypes | string) =>
['treemap', 'mosaic'].includes(shape);

export const shouldShowValuesInLegend = (layer: PieLayerState, shape: PieChartTypes) =>
layer.showValuesInLegend !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't checking the current sub type (pie, treemap, ...), it's possible to get into a weird state by enabling values in waffle, then switching to pie (without being able to disable it again):
Screenshot 2021-12-06 at 19 08 52

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha.... good point, I've missed that. Thank you so much

flash1293
flash1293 reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for the weird state transition

@alexwizp
Copy link
Contributor Author

alexwizp commented Dec 7, 2021

@elasticmachine merge upstream

@alexwizp alexwizp requested a review from mbondyra December 7, 2021 11:12
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #1 / APM API tests basic apm_8.0.0 Instance details when data is loaded fetch instance details return the correct data

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
lens 243 244 +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 969.6KB 970.3KB +723.0B

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.3KB 40.4KB +47.0B
Unknown metric groups

API count

id before after diff
lens 260 261 +1

History

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

cc @alexwizp

mbondyra
mbondyra approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Chrome and works as expected. It preserves the setting when switching between partition charts, but doesn't have impact on other types so I think it is fine 👌🏼

@alexwizp alexwizp merged commit 4d2f618 into elastic:main Dec 7, 2021
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
@alexwizp @kibanamachine @TinLe
[Lens] Waffle - add "show values in legend" configuration property (e…
a0ea27a 
…lastic#120471)

* [Lens] Mosaic - add show values in legend configuration property for Waffle chart type

* add tests

* resolve comments

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghudgins ghudgins ghudgins left review comments

@flash1293 flash1293 flash1293 left review comments

@mbondyra mbondyra mbondyra approved these changes

@VladLasitsa VladLasitsa Awaiting requested review from VladLasitsa

Assignees

@alexwizp alexwizp

Labels
backport:skip This commit does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alexwizp @elasticmachine @kibana-ci @flash1293 @mbondyra @ghudgins @kibanamachine