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

Embeddable input #73033

Merged
merged 4 commits into from
Aug 21, 2020
Merged

Embeddable input #73033

merged 4 commits into from
Aug 21, 2020

Conversation

streamich
Copy link
Contributor

Summary

Closes #72826

Moves core embeddable types on base interface from visualize embeddable interface.

@streamich streamich requested a review from a team July 23, 2020 12:09
@streamich streamich requested a review from a team as a code owner July 23, 2020 12:09
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Jul 23, 2020
@Dosant
Copy link
Contributor

Dosant commented Jul 23, 2020 •
edited
Loading

In this pr I removed explicit dependency from embeddable to data, but current pr makes decencies a bit worse?
At least good that it is just types, let's explicitly use #73033 (comment) to not create accidentally unwanted runtime dependency later?

I am ok with merging this, but I wonder if this is a bad thing that BaseEmbeddable interface has interfaces from data plugin? (even though they are optional)

Maybe we should have data_embeddable plugin which would have DateEmbeddable extends IEmbeddable and some other helpers around that area. And then visualise would depend on it?

@streamich streamich added the WIP Work in progress label Jul 24, 2020
ppisljar
ppisljar approved these changes Jul 28, 2020
View reviewed changes
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 added Team:AppArch v7.10.0 v8.0.0 and removed WIP Work in progress labels Aug 21, 2020
@elasticmachine
Copy link
Contributor

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

@streamich streamich added the release_note:skip Skip the PR/issue when compiling release notes label Aug 21, 2020
@streamich
Copy link
Contributor Author

@Dosant Chatted with Peter, agreed that it is ok to import types from data plugin. Also, we can consider adding a new data_embeddable plugin later if we really think it is worth it.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
discoverEnhanced 29.2KB -210.0B 29.4KB

History

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

@streamich streamich merged commit 74ab989 into elastic:master Aug 21, 2020
@streamich streamich mentioned this pull request Aug 21, 2020
Merged
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 21, 2020
@gmmorris
Merge branch 'master' into actions/webhook-remove-header
452f964 
* master: (71 commits)
  [Lens] Show 'No data for this field' for empty field in accordion (elastic#73772)
  Skip failing lens test
  Configure ScopedHistory consistenty regardless of URL used to mount app (elastic#75074)
  Fix returned payload by "search" usage collector (elastic#75340)
  [Security Solution] Fix missing key error (elastic#75576)
  Upgrade EUI to v27.4.1 (elastic#75240)
  Update datasets UI copy to data streams (elastic#75618)
  [Lens] Register saved object references (elastic#74523)
  [DOCS] Update links to Beats documentation (elastic#70380)
  [Enterprise Search] Convert our `public_url` route to `config_data` and collect initialAppData (elastic#75616)
  [Usage Collection Schemas] Remove Legacy entries (elastic#75652)
  [Dashboard First] Lens Originating App Breadcrumb (elastic#75470)
  Improve login UI error message. (elastic#75642)
  [Security Solution] modify circular deps checker to output images of circular deps graphs (elastic#75579)
  [Data Telemetry] Add index pattern to identify "meow" attacks (elastic#75163)
  Migrate CSP usage collector to `kibana_usage_collection` plugin (elastic#75536)
  [Console] Get ES Config from core (elastic#75406)
  [Uptime] Add delay in telemetry test (elastic#75162)
  [Lens] Use index pattern service instead saved object client (elastic#74654)
  Embeddable input (elastic#73033)
  ...
thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Aug 21, 2020
@streamich @thomasneirynck
Embeddable input (elastic#73033)
42c2f8e 
* refactor: 💡 move timeRange, filters and query to base embeddabl

* refactor: 💡 use new base embeddable input in explore data

* feat: 🎸 import types as types
streamich added a commit that referenced this pull request Aug 22, 2020
@streamich
Embeddable input (#73033) (#75630)
2f5546c 
* refactor: 💡 move timeRange, filters and query to base embeddabl

* refactor: 💡 use new base embeddable input in explore data

* feat: 🎸 import types as types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dosant Dosant Dosant left review comments

@majagrubic majagrubic majagrubic approved these changes

@ppisljar ppisljar ppisljar approved these changes

@kertal kertal Awaiting requested review from kertal

@ThomThomson ThomThomson Awaiting requested review from ThomThomson

Assignees
No one assigned
Labels
Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

update base embeddable input
6 participants
@streamich @Dosant @elasticmachine @kibanamachine @majagrubic @ppisljar