Skip to content

Conversation

@streamich
Copy link
Contributor

@streamich streamich commented Aug 6, 2020

Closes #71848

  • Moves "render-complete" out of VisualizeEmbeddable into Embeddable and RenderCompleteDispatcher. This allows us to add render-complete logic to any embeddable. And because it is separated out into RenderCompleteDispatcher we can also reuse that logic outside of embeddables.

P.S. Render-complete logic is needed for reporting and functional tests, so they know elements on the page have loaded and finished rendering.

@streamich streamich requested a review from a team August 6, 2020 12:06
@streamich streamich requested a review from a team as a code owner August 6, 2020 12:06
@streamich streamich added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Aug 6, 2020
@wylieconlon
Copy link
Contributor

Is this ready to review? It's unclear what this is about based on the description and tags

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

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

Hey @wylieconlon, sorry I was on holidays. I uploaded this PR last week to see if build succeeds, but now it is ready for review. I've liked the correct issue in description and will update the tags.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

Copy link
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@streamich streamich mentioned this pull request Aug 13, 2020
33 tasks
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
kibanaUtils 191 +1 190

async chunks size

id value diff baseline
discover 430.7KB +6.0B 430.7KB

page load bundle size

id value diff baseline
embeddable 429.9KB +1.1KB 428.7KB
kibanaUtils 472.8KB +2.3KB 470.6KB
visualizations 408.3KB -689.0B 409.0KB
total +2.7KB

History

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

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, CodeOwner review, tested locally in Chrome, Firefox, MacOs, data-render-complete of Discover works 👍

@streamich streamich merged commit cd36188 into elastic:master Aug 20, 2020
streamich added a commit that referenced this pull request Aug 20, 2020
* chore: 🤖 remove unused render-complete logic

* feat: 🎸 add render-complete observables to IEmbeddable

* Revert "chore: 🤖 remove unused render-complete logic"

This reverts commit 0049c01.

* refactor: 💡 rename render complete "helper" to "listener"

* feat: 🎸 add render complete dispatcher to embeddable

* refactor: 💡 move data-title setup to Embeddable

* refactor: 💡 move embeddable data-title setting to render-compl

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/visualizations/public/embeddable/visualize_embeddable.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes review v7.10.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RenderComplete helper (visualize_embeddable)

6 participants