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

[Canvas] Expression progress #104457

Merged
merged 15 commits into from Aug 4, 2021
Merged

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented Jul 6, 2021

Completes a part of #101377.

At this PR progress expression is extracted from the canvas plugin and set up as a separate plugin.

List of required changes to be done:

  • Extract progress expression from canvas to a separate plugin.
  • Move it to ts and React.
  • Add Storybook for the progress expression renderer.
  • Remove legacy expression from canvas plugin
  • Add fixes of errors

Testing Notes

This moves the progress expression function to a standalone plugin and refactors the code. To test, just test that the progress expression in canvas continues to work as expected.

@Kuznietsov Kuznietsov added release_note:enhancement WIP Work in progress Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Feature:Canvas auto-backport Deprecated: Automatically backport this PR after it's merged loe:weeks v7.15.0 labels Jul 6, 2021
@Kuznietsov Kuznietsov requested a review from alexwizp July 6, 2021 14:05
@Kuznietsov Kuznietsov self-assigned this Jul 14, 2021
@Kuznietsov Kuznietsov removed the WIP Work in progress label Jul 14, 2021
@Kuznietsov Kuznietsov changed the title [WIP][Canvas] Expression progress [Canvas] Expression progress Jul 14, 2021
@Kuznietsov Kuznietsov mentioned this pull request Jul 16, 2021
5 tasks
@Kuznietsov Kuznietsov requested a review from crob611 July 27, 2021 09:37
@Kuznietsov Kuznietsov marked this pull request as ready for review July 27, 2021 09:37
@Kuznietsov Kuznietsov requested review from a team as code owners July 27, 2021 09:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Everything looks good. Need to figure out those class names that start with "canvas"

@@ -1,8 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
* 2.0 and the Server Side Public License, v 1; you may not use this file except
Copy link
Contributor

Choose a reason for hiding this comment

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

What caused the need to move this file to pres_util?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to try to keep presentation_util as only things that are really presentation related. Can we just inline the id generation on the SVG?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated PR based on your nit.

const BarProgress = shapeData.shapeType ? getShapeContentElement(shapeData.shapeType) : null;

const shapeContentAttributes = {
className: 'canvasProgress__background',
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to get rid of all these canvas_ prefixed class names since this is not strictly canvas anymore. Are they even used for anything at this point? Can you double check on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this canvas* classes. According to the code, it is not necessary, but, possibly, some tests were using it. After passing the CI we'll see)))

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
@Kuznietsov Kuznietsov requested a review from crob611 July 28, 2021 13:49
@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@crob611 crob611 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 to go 👍

@Kuznietsov Kuznietsov merged commit dbab7d9 into elastic:master Aug 4, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 104457

Kuznietsov added a commit to Kuznietsov/kibana that referenced this pull request Aug 4, 2021
* Added `expression_progress` plugin.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	packages/kbn-optimizer/limits.yml
Kuznietsov added a commit that referenced this pull request Aug 4, 2021
* [Canvas] Expression progress (#104457)

* Added `expression_progress` plugin.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	packages/kbn-optimizer/limits.yml

* Update limits.yml
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
* Added `expression_progress` plugin.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
canvas 1057 1045 -12
expressionShape 42 61 +19
total +7

Async chunks

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

id before after diff
canvas 1.6MB 1.6MB -10.1KB
expressionShape 7.3KB 23.5KB +16.2KB
total +6.1KB

Page load bundle

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

id before after diff
expressionShape 17.3KB 27.5KB +10.2KB
Unknown metric groups

API count

id before after diff
expressionShape 90 141 +51

API count missing comments

id before after diff
expressionShape 90 141 +51

async chunk count

id before after diff
expressionShape 2 4 +2

References to deprecated APIs

id before after diff
canvas 36 34 -2

History

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

cc @Kunzetsov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Canvas impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants