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

[PieVis] Chart expressions pie. #121612

Merged
merged 25 commits into from
Jan 4, 2022
Merged

[PieVis] Chart expressions pie. #121612

merged 25 commits into from
Jan 4, 2022

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented Dec 20, 2021 •
edited
Loading

Summary

Completes a part of #107500.

At this PR pieVis function and renderer have been moved to chart_expressions/expression_pie.

@Kuznietsov Kuznietsov added the WIP Work in progress label Dec 20, 2021
@Kuznietsov Kuznietsov self-assigned this Dec 20, 2021
@Kuznietsov Kuznietsov marked this pull request as ready for review December 20, 2021 15:14
@Kuznietsov Kuznietsov requested review from a team as code owners December 20, 2021 15:14
@Kuznietsov Kuznietsov added Feature:Pie Chart Pie chart visualization feature Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Dec 20, 2021
@elasticmachine
Copy link
Contributor

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

@Kuznietsov Kuznietsov added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort and removed WIP Work in progress impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Dec 20, 2021
@Kuznietsov Kuznietsov added backport:skip This commit does not require backporting v8.1.0 Feature:Pie Chart Pie chart visualization feature labels Dec 20, 2021
@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elastic/kibana-vis-editors, folks, could you, please, review the current PR?) Thanks a lot )

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elastic/kibana-vis-editors, folks, could you, please, review the current PR?) Thanks a lot )

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

stratoula
stratoula reviewed Jan 4, 2022
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

This works great. It hasn't changed the api at all, we just moving files around. I only have two comments to remove things that are not needed.

src/plugins/vis_types/pie/public/plugin.ts
@@ -45,24 +42,10 @@ export interface VisTypePieDependencies {
export class VisTypePiePlugin {
setup(
core: CoreSetup<VisTypePiePluginStartDependencies>,
{ expressions, visualizations, charts, usageCollection }: VisTypePieSetupDependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also remove the expressions from the VisTypePieSetupDependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, sorry) Done.

src/plugins/chart_expressions/expression_pie/public/plugin.ts Outdated
export class ExpressionPiePlugin {
public setup(
core: CoreSetup<VisTypePiePluginStartDependencies>,
{ expressions, visualizations, charts, usageCollection }: SetupDeps
Copy link
Contributor

Choose a reason for hiding this comment

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

The visualizations and usageCollection are not used here, let's remove them. We can also remove the usageCollection from the kibana.json required plugins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks. Done)

stratoula
stratoula approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Thanks for applying my comments! Code changes LGTM, I tested it on Chrome and works fine. Please feel free to merge in case of green CI

@Kuznietsov
Copy link
Contributor Author

@stratoula, thanks a lot)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
expressionPie - 42 +42
visTypePie 48 14 -34
total +8

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
expressionPie - 43 +43

Async chunks

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

id before after diff
expressionPie - 43.3KB +43.3KB
visTypePie 51.5KB 8.4KB -43.1KB
total +143.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
expressionPie - 2 +2
visTypePie 2 1 -1
total +1

Page load bundle

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

id before after diff
expressionPie - 11.0KB +11.0KB
visTypePie 14.4KB 7.8KB -6.6KB
total +4.4KB
Unknown metric groups

API count

id before after diff
expressionPie - 43 +43

async chunk count

id before after diff
expressionPie - 1 +1
visTypePie 2 1 -1
total -0

ESLint disabled line counts

id before after diff
expressionPie - 1 +1
visTypePie 2 1 -1
total -0

References to deprecated APIs

id before after diff
expressionPie - 15 +15
visTypePie 17 0 -17
total -2

Total ESLint disabled count

id before after diff
expressionPie - 1 +1
visTypePie 2 1 -1
total -0

History

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

cc @Kunzetsov

@Kuznietsov Kuznietsov merged commit 6046f46 into elastic:main Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@MichaelMarcialis MichaelMarcialis MichaelMarcialis approved these changes

@jbudz jbudz jbudz approved these changes

@crob611 crob611 Awaiting requested review from crob611

@flash1293 flash1293 Awaiting requested review from flash1293

@alexwizp alexwizp Awaiting requested review from alexwizp

Assignees

@Kuznietsov Kuznietsov

Labels
backport:skip This commit does not require backporting Feature:Pie Chart Pie chart visualization feature impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort 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
@Kuznietsov @elasticmachine @kibana-ci @jbudz @MichaelMarcialis @stratoula @kibanamachine