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 repeat image #104255

Merged
merged 13 commits into from
Jul 23, 2021

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented Jul 2, 2021

Completes a part of #101377.
Fixes: #97087

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

List of required changes to be done:

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

Testing Notes

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

@Kuznietsov Kuznietsov requested a review from alexwizp July 2, 2021 15:27
@Kuznietsov Kuznietsov added the WIP Work in progress label Jul 2, 2021
@Kuznietsov Kuznietsov changed the title [Canvas] Expression repeat image [WIP][Canvas] Expression repeat image Jul 2, 2021
@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

  • 💔 Build #136224 failed 7f7221c7883b0bc36d1b544c47d742f7c42c8c5b

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

@Kuznietsov Kuznietsov changed the title [WIP][Canvas] Expression repeat image [Canvas] Expression repeat image Jul 5, 2021
@Kuznietsov Kuznietsov added auto-backport Deprecated - use backport:version if exact versions are needed Feature:Canvas impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:weeks release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.15.0 v8.0.0 and removed WIP Work in progress labels Jul 5, 2021
@Kuznietsov
Copy link
Contributor Author

@elasticmachine merge upstream

@Kuznietsov Kuznietsov self-assigned this Jul 21, 2021
@Kuznietsov Kuznietsov force-pushed the expression_repeat_image branch from 530f36a to dd5f20f Compare July 22, 2021 09:20
@Kuznietsov Kuznietsov marked this pull request as ready for review July 22, 2021 10:02
@Kuznietsov Kuznietsov requested review from a team as code owners July 22, 2021 10:02
@elasticmachine
Copy link
Contributor

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

@Kuznietsov Kuznietsov requested a review from crob611 July 22, 2021 10:03
# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml and storybook aliases LGTM

@Kuznietsov
Copy link
Contributor Author

@jbudz, thank you for the review )

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 great. A few small nits to fix before merging.

* Side Public License, v 1.
*/

import { ExpressionMetricPlugin } from './plugin';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be ExpressionRepeatImagePlugin. Probably just a copy/paste remnant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

implements
Plugin<ExpressionMetricPluginSetup, ExpressionMetricPluginStart, SetupDeps, StartDeps> {
public setup(core: CoreSetup, { expressions }: SetupDeps): ExpressionMetricPluginSetup {
expressions.registerRenderer(repeatImageRenderer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also register the function here so that it is available both client and server side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Kuznietsov
Copy link
Contributor Author

@crob611, thanks for your review)

# Conflicts:
#	x-pack/plugins/translations/translations/zh-CN.json
@Kuznietsov Kuznietsov merged commit 3e4b64b into elastic:master Jul 23, 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 104255

Kuznietsov added a commit to Kuznietsov/kibana that referenced this pull request Jul 23, 2021
* Repeat Image plugin added.
# Conflicts:
#	packages/kbn-optimizer/limits.yml
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
Kuznietsov added a commit that referenced this pull request Jul 23, 2021
* [Canvas] Expression repeat image (#104255)

* Repeat Image plugin added.
# Conflicts:
#	packages/kbn-optimizer/limits.yml
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json

* Removed not defined plugin `userSetup`.
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
canvas 1002 999 -3
expressionRepeatImage - 10 +10
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.5MB 1.5MB -4.8KB
expressionRepeatImage - 2.4KB +2.4KB
total -2.4KB

Page load bundle

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

id before after diff
canvas 24.3KB 24.5KB +248.0B
expressionRepeatImage - 10.1KB +10.1KB
total +10.3KB
Unknown metric groups

API count

id before after diff
expressionRepeatImage - 28 +28

API count missing comments

id before after diff
expressionRepeatImage - 28 +28

async chunk count

id before after diff
expressionRepeatImage - 1 +1

References to deprecated APIs

id before after diff
canvas 55 53 -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 - use backport:version if exact versions are needed 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.

[Canvas] Image Repeat element - the number of images becomes incorrect when the value changes
5 participants