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.

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

[Lens] Pie and treemap charts #55477

Merged
merged 57 commits into from
May 5, 2020
Merged

Conversation

wylieconlon
Copy link
Contributor

@wylieconlon wylieconlon commented Jan 21, 2020

Screenshots for reference:

Screenshot 2020-05-04 17 56 35 Screenshot 2020-05-04 17 53 33
Screenshot 2020-05-04 17 53 59 Screenshot 2020-05-04 17 57 17

Chart switcher prevents you from switching to Treemap if there are too many layers:

Screenshot 2020-05-04 18 09 02

Closes #38837

Checklist

@wylieconlon wylieconlon changed the title [Lens] Pie/sunburst chart [Lens] Donut/sunburst/treemap chart Jan 23, 2020
@wylieconlon wylieconlon added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 28, 2020
@elasticmachine
Copy link
Contributor

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

@wylieconlon wylieconlon added this to In progress in Lens via automation Jan 28, 2020
@wylieconlon

This comment has been minimized.

@wylieconlon

This comment has been minimized.

@AlonaNadler

This comment has been minimized.

@wylieconlon

This comment has been minimized.

@AlonaNadler

This comment has been minimized.

@wylieconlon

This comment has been minimized.

@monfera

This comment has been minimized.

@monfera

This comment has been minimized.

@wylieconlon

This comment has been minimized.

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

}
const metricColumn = firstTable.columns.find(c => c.id === metric)!;
const percentFormatter =
metricColumn.formatHint && metricColumn.formatHint?.id === 'percent'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
metricColumn.formatHint && metricColumn.formatHint?.id === 'percent'
metricColumn?.formatHint?.id === 'percent'

@wylieconlon

This comment has been minimized.

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.

I left some feedback in private messages about functional behaviour, thanks for creating a todo list!

@wylieconlon

This comment has been minimized.

@wylieconlon wylieconlon requested review from timroes, mbondyra and a team and removed request for a team May 5, 2020 03:28
@mbondyra
Copy link
Contributor

mbondyra commented May 5, 2020

Tested and all works as expected! The only thing that could use a bit of improvement would be the looks of treemap. The lack of padding between labels and chart slices looks a bit 'unfinished' to me, but that's not a blocker for releasing :)

image

@markov00
Copy link
Member

markov00 commented May 5, 2020

Tested and all works as expected! The only thing that could use a bit of improvement would be the looks of treemap. The lack of padding between labels and chart slices looks a bit 'unfinished' to me, but that's not a blocker for releasing :)

image

We are going to release a fix for that padding today

@AlonaNadler
Copy link

Hey, really nice job!!!

Any idea why when doing average it only shows one area and on sum, it shows more sub-areas. does it make sense or is it a bug?

May-05-2020 09-20-38

The suggestions with the pie chart work well. the pie suggestions seem to be instead of table suggestions. There is still more place in the suggestion to include the table. Any chance you can add the table as well?

something I noticed, not sure if it's only in this or not
May-05-2020 09-27-42

@wylieconlon
Copy link
Contributor Author

wylieconlon commented May 5, 2020

@AlonaNadler If there's data missing, it's probably because it has 0s: because they have 0 area, they are hidden. Table suggestions were removed in #64740, and it doesn't make sense to bring them back in this PR.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Did some scans and played with the three different chart types. Pretty slick.

One small thing to follow up with, but should go in for 7.8, is to update the "Label position" options for Treemaps. As "Inside or outside" and "Inside only" don't change anything and will be confusing/seem buggy to users.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@wylieconlon wylieconlon merged commit e5d7bb6 into elastic:master May 5, 2020
Lens automation moved this from In progress to Done May 5, 2020
wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request May 5, 2020
* [Lens] Add pie and treemap visualizations

* Fix types

* Update to new platform

* Support 2-layer treemap and legends, dark mode

* Significant restructuring of code

* Upgrade to latest charts library

* Commit yarn.lock

* chore: update elastic-charts

* fix types after merge master

* Add settings panel and merge visualizations

* Fix tests

* build: upgrade @elastic/charts to 19.0.0

* refactor: onBrushEnd breaking changes

* fix: missing onBrushEnd argument changes

* More updates

* Fix XY rendering issue when all dates are empty

* Fix bugs and tests

* Use shared services location

* Fix bug in XY chart

* fix: update ech to 19.1.1

* fix: lens onBrushEnd breaking changes

* Change how pie/treemap settings work

* [Design] Fix up settings panel

* [Design] Update partition chart config styles

* fix eslint

* Fix legend issues in pie and XY, add some tests

* update to 19.1.2

* Fix text color for treemap

* Fix chart switch bug

* Fix suggestions

Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>
@wylieconlon wylieconlon deleted the lens/pie branch May 5, 2020 20:01
wylieconlon pushed a commit that referenced this pull request May 5, 2020
* [Lens] Add pie and treemap visualizations

* Fix types

* Update to new platform

* Support 2-layer treemap and legends, dark mode

* Significant restructuring of code

* Upgrade to latest charts library

* Commit yarn.lock

* chore: update elastic-charts

* fix types after merge master

* Add settings panel and merge visualizations

* Fix tests

* build: upgrade @elastic/charts to 19.0.0

* refactor: onBrushEnd breaking changes

* fix: missing onBrushEnd argument changes

* More updates

* Fix XY rendering issue when all dates are empty

* Fix bugs and tests

* Use shared services location

* Fix bug in XY chart

* fix: update ech to 19.1.1

* fix: lens onBrushEnd breaking changes

* Change how pie/treemap settings work

* [Design] Fix up settings panel

* [Design] Update partition chart config styles

* fix eslint

* Fix legend issues in pie and XY, add some tests

* update to 19.1.2

* Fix text color for treemap

* Fix chart switch bug

* Fix suggestions

Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>

Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lens] Implement pie chart visualization
9 participants