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

[Controls] [Dashboard] Allow existing controls to change type #129385

Merged
merged 17 commits into from
May 12, 2022

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Apr 4, 2022

Closes #128195

Summary

This PR introduces the ability to replace an existing control embeddable with one of a different type via the new container function replaceEmbeddable.

While the replaceEmbeddable function is designed to be a generic function that could apply to other embeddables, for the sake of keeping this PR simple, I only used it for controls for now - in future PRs, we could use this replaceEmbeddable function in other places, such as in the replacePanel code in the Dashboard container (dashboard_container.ts) to reduce duplicated code.

Video

2022-05-02_ReplaceControlType.mp4

Flaky Test Runner

image

Checklist

For maintainers

@Heenawter Heenawter added Feature:Dashboard Dashboard related features Feature:Input Control Input controls visualization enhancement New value added to drive a business result Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:large Large Level of Effort loe:x-large Extra Large Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. backport:skip This commit does not require backporting v8.3.0 labels Apr 4, 2022
@Heenawter Heenawter self-assigned this Apr 4, 2022
@Heenawter Heenawter force-pushed the allow-control-type-change_2022-03-28 branch 3 times, most recently from e7ce8a1 to af7e998 Compare April 6, 2022 17:19
@Heenawter Heenawter force-pushed the allow-control-type-change_2022-03-28 branch from af7e998 to dca35f2 Compare April 20, 2022 15:42
@Heenawter Heenawter force-pushed the allow-control-type-change_2022-03-28 branch 5 times, most recently from 70db39d to 54bf146 Compare April 21, 2022 21:28
@Heenawter Heenawter force-pushed the allow-control-type-change_2022-03-28 branch from 54bf146 to 7a7fdac Compare May 2, 2022 16:09
@Heenawter Heenawter force-pushed the allow-control-type-change_2022-03-28 branch from 5bcdf06 to 26f84ec Compare May 2, 2022 16:47
@Heenawter Heenawter marked this pull request as ready for review May 2, 2022 18:09
@Heenawter Heenawter requested review from a team as code owners May 2, 2022 18:09
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label May 9, 2022
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

This is looking great! Left one DX / style comment, and a couple questions, but I think this is overall very well done! As I said in our sync earlier, I can't wait to use this code to simplify all the other places where embeddable panels get swapped out. This could go a long way towards making our containers more responsive.

Also, it looks as though you've accomplished this while keeping the same embeddable id, which is super cool.

throw new EmbeddableFactoryNotFoundError(newType);
}

newExplicitInput.id = id;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to avoid mutation here?

Copy link
Contributor Author

@Heenawter Heenawter May 12, 2022

Choose a reason for hiding this comment

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

@tsullivan Good catch! I did it this way instead:

this.updateInput({
  panels: {
      ...
      explicitInput: { ...newExplicitInput, id },
      ...

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

embeddable Code lgtm,
I also tested in an example plugin that updating input of dashboard controller panel with a different type/input properly replaces it ( which matches internal implementation of the new replace method)

Does this replace functionality covered by functional tests? I assume so and looks like yes

@Heenawter
Copy link
Contributor Author

@Dosant Yes, I added functional tests to test this! :) Only for controls for now, but we can expand the other replace-related functional tests and the related Jest tests in a follow up PR that will make use of this new functionality in other places, such as panel cloning - I felt as though it was best to keep all changes in this PR as contained within control as possible 👍

@Heenawter Heenawter force-pushed the allow-control-type-change_2022-03-28 branch from e528126 to 9c55499 Compare May 12, 2022 17:24
@Heenawter Heenawter enabled auto-merge (squash) May 12, 2022 17:45
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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
controls 201 204 +3
embeddable 388 396 +8
total +11

Async chunks

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

id before after diff
controls 420.3KB 420.8KB +482.0B
presentationUtil 127.3KB 127.4KB +46.0B
total +528.0B

Page load bundle

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

id before after diff
controls 32.0KB 31.8KB -204.0B
embeddable 65.9KB 66.2KB +387.0B
total +183.0B
Unknown metric groups

API count

id before after diff
controls 207 210 +3
embeddable 478 486 +8
total +11

History

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

cc @Heenawter

@Heenawter Heenawter merged commit 595522b into elastic:main May 12, 2022
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request May 16, 2022
…c#129385)

* Replace is half working

* Child embeddable listens for change in type

* Fix control order on replace

* Fix factory & remove duplicated onPanelAdded/Removed

* Comments + clean up code

* Set invalid editor state on type change

* Add functional tests and clean test code

* Comment out time slider tests

* Remove getReplacementPanelState

* Fix promise syntax

* Fix mutation

* Fix flaky test

* Fix conflicts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting enhancement New value added to drive a business result Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Feature:Input Control Input controls visualization impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort loe:x-large Extra Large Level of Effort Project:Controls release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Controls] Allow the control type to be changed when editing existing control
6 participants