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

ui/render_complete 👉 src/plugins/kibana_utils #44367

Closed
4 tasks done
streamich opened this issue Aug 29, 2019 · 3 comments · Fixed by #44743
Closed
4 tasks done

ui/render_complete 👉 src/plugins/kibana_utils #44367

streamich opened this issue Aug 29, 2019 · 3 comments · Fixed by #44743
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available)

Comments

@streamich
Copy link
Contributor

streamich commented Aug 29, 2019

  • ui/render_complete 👉 src/plugins/kibana_utils
    • Move src/legacy/ui/public/render_complete/index.ts
    • Move src/legacy/ui/public/render_complete/render_complete_helper.ts
    • Keep src/legacy/ui/public/render_complete/directive.js, check it still works

Parent issue: #44121

@streamich streamich added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Team:AppArch labels Aug 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@stacey-gammon
Copy link
Contributor

I wonder if we need render complete helper at all, if we moved this logic inside embeddable api. Only problem is that we need to pass down a fn through many visualize layer to get it to the right spot which I think is what this render complete helper helps us avoid.

visualize_embeddable.ts:

 setRender(status: OneOf<RENDER_COMPLETE, RENDERING>) {
  this.updateOutput({ renderStatus: status });
 }

 render() {
   visualizeLoader({... 
     // Somehow we'd have to pass this function down through many layer to have it available in all of the different implementations that need to indicate "render complete".
     setRender: this.setRender
  }   
 } 

One way that's previous been discussed to avoid passing down a fn through many layer is to use triggers... but we still need an id of the embeddable so it knows how to map the caller to itself.

  VisualizeEmbeddable {
    constructor() {..
      this.attachAction(RENDER_UPDATE_TRIGGER, {
        execute: (embeddableId: string, renderStatus: RenderStatus) => {
          if (this.id === embeddableId) {
            this.updateOutput({ renderStatus });
          }
        }
    }
  }

Not sure how else but embeddable id we could match the emitter of the trigger to the embeddable that should update its status.

But if there is no easy fix right now in embeddables... do we really want to block np migration, or just barge ahead even knowing that this code should probably go away at some point.

@mattkime
Copy link
Contributor

mattkime commented Sep 6, 2019

I agree that there might be a better way to organize this code but I'm still rather new to it but I don't have a recommendation. We should resolve this in v7.5 but we have some time to consider our options and doing other tasks might help clarify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants