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

[Dashboard] Fix cloning panels reactive issue #74253

Merged
merged 15 commits into from
Nov 6, 2020
Merged

[Dashboard] Fix cloning panels reactive issue #74253

merged 15 commits into from
Nov 6, 2020

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Aug 4, 2020 •
edited
Loading

This PR aims to fix the panel cloning behaviour in the dashboard context, without creating new ids.

The main change has been implemented in the diff function maybeUpdateChildren which now receives the previous and current state and can decide to update based on the content difference (only type as for now).

Fixes #70381

@dej611 dej611 changed the title Fix for embeddable panel input for dashboard [Dashboard] Fix cloning panels reactive issue Aug 4, 2020
@dej611 dej611 added Feature:Dashboard Dashboard related features Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0 labels Aug 4, 2020
@ThomThomson ThomThomson mentioned this pull request Aug 4, 2020
Closed
@dej611 dej611 self-assigned this Aug 5, 2020
@dej611 dej611 marked this pull request as ready for review August 17, 2020 15:42
@dej611 dej611 requested a review from a team August 17, 2020 15:42
@dej611 dej611 requested a review from a team as a code owner August 17, 2020 15:42
@elasticmachine
Copy link
Contributor

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

@ThomThomson ThomThomson mentioned this pull request Aug 17, 2020
Merged
2 tasks
ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Aug 17, 2020
@ThomThomson
Merged elastic#72256 and elastic#74253 in order to test panel reactiv…
9562949 
…ity for by value embeddables
@ThomThomson ThomThomson mentioned this pull request Aug 17, 2020
Closed
ThomThomson
ThomThomson reviewed Aug 17, 2020
View reviewed changes
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.

@dej611, After taking a look, I think the success we were seeing earlier was a false positive cause by editing a 'by reference' embeddable without knowing. I merged in #72256 in order to test reactivity after editing a by value visualization, and the changes still didn't show up until the page was hard reloaded. Additionally, cloning seems to be broken for 'by value' embeddables.

If you want to take a look at the combined code, here's a PR with them together: #75222

At this point, my suggestion would be to revisit this after #70272 and #72256 are merged, so that it's easier to test. Additionally, we're considering adding a 'by reference' badge to embeddables that are saved in the library. That will also make it easier to test this code.

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 17, 2020
@dej611
Copy link
Contributor Author

dej611 commented Aug 18, 2020

Yes, agree. It makes sense to check all the scenarios at once and solve them, rather than chasing each one during their development.

@dej611 dej611 marked this pull request as draft August 26, 2020 16:16
@ThomThomson
Copy link
Contributor

Now may be a good time to revisit this problem, considering that the Lens & Visualize by Value PRs have been merged. It's still important to clean up this behaviour!

@dej611 dej611 removed the v7.10.0 label Nov 4, 2020
@dej611
Copy link
Contributor Author

dej611 commented Nov 5, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
dashboard 311.5KB 311.4KB -98.0B
embeddable 220.6KB 221.1KB +536.0B
total +438.0B

History

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

ThomThomson
ThomThomson approved these changes Nov 5, 2020
View reviewed changes
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.

Tested locally in Chrome, everything still works as expected. Code LGTM, tests looking great. Overall, great fix & super exciting change!

@dej611 dej611 marked this pull request as ready for review November 5, 2020 17:57
@dej611 dej611 requested a review from a team as a code owner November 5, 2020 17:57
@dej611 dej611 mentioned this pull request Nov 6, 2020
Closed
Dosant
Dosant approved these changes Nov 6, 2020
View reviewed changes
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.

LGTM

I tested using dashboard container by value developer example. Now I can change types of embeddable in embeddable input and get an expected result 👍

There is an existing functional test we can also improve by covering this edge case: https://github.com/elastic/kibana/blob/master/test/examples/embeddables/dashboard.ts

@dej611
Copy link
Contributor Author

dej611 commented Nov 6, 2020

There is an existing functional test we can also improve by covering this edge case: master/test/examples/embeddables/dashboard.ts

Great. I'll submit a follow up PR with the test

@dej611 dej611 added the v7.11.0 label Nov 6, 2020
@dej611 dej611 merged commit 1b65a67 into elastic:master Nov 6, 2020
@dej611 dej611 deleted the fix/embeddable-panel-input-updates branch November 6, 2020 10:17
dej611 added a commit to dej611/kibana that referenced this pull request Nov 6, 2020
@dej611 @kibanamachine
[Dashboard] Fix cloning panels reactive issue (elastic#74253)
74ab724 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@dej611 dej611 mentioned this pull request Nov 6, 2020
Merged
dej611 added a commit that referenced this pull request Nov 6, 2020
@dej611 @kibanamachine
[7.x] [Dashboard] Fix cloning panels reactive issue (#74253) (#82812)
29be25d 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
This was referenced Nov 19, 2020
Closed
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dosant Dosant Dosant approved these changes

@ThomThomson ThomThomson ThomThomson approved these changes

Assignees

@dej611 dej611

Labels
Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Embeddable Panel is not reactive to Input Updates
5 participants
@dej611 @elasticmachine @ThomThomson @kibanamachine @Dosant