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

Modularize render complete #74504

Merged
merged 9 commits into from
Aug 20, 2020
Merged

Modularize render complete #74504

merged 9 commits into from
Aug 20, 2020

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
Member

@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