Skip to content

cleaning up embeddable types#75560

Merged
ppisljar merged 7 commits intoelastic:masterfrom
ppisljar:embeddable/types
Aug 25, 2020
Merged

cleaning up embeddable types#75560
ppisljar merged 7 commits intoelastic:masterfrom
ppisljar:embeddable/types

Conversation

@ppisljar
Copy link
Contributor

@ppisljar ppisljar commented Aug 20, 2020

Summary

embeddable input had this [key: string]: unknown; property, which is too loosely typed.

a few places need to be cleaned up, mostly related to saved object embeddables and cases where the proper input type was not used.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ppisljar ppisljar requested review from a team and stacey-gammon August 20, 2020 14:10
@ppisljar ppisljar requested a review from a team as a code owner August 20, 2020 14:10
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 20, 2020
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added the Feature:Drilldowns Embeddable panel Drilldowns label Aug 20, 2020
@ThomThomson ThomThomson added v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 20, 2020
@ppisljar
Copy link
Contributor Author

@elasticmachine merge upstream


if (context.embeddable) {
const input = context.embeddable.getInput();
const input = context.embeddable.getInput() as Readonly<SearchInput>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ThomThomson why Readonly<SearchInput> ? shouldn't query, filters and range be on embeddableinput directly ?
cc @streamich

Copy link
Contributor

Choose a reason for hiding this comment

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

They will be, I'll merge that PR that adds query, filters and range onto base embeddable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please ping here once that is done and i can merge master

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

# Conflicts:
#	src/plugins/embeddable/public/lib/embeddables/i_embeddable.ts
#	x-pack/plugins/discover_enhanced/public/actions/explore_data/explore_data_context_menu_action.ts
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.

Changes LGTM! Very excited to get rid of this, and make typings a little stricter.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@ppisljar ppisljar merged commit 40d8edc into elastic:master Aug 25, 2020
ppisljar added a commit to ppisljar/kibana that referenced this pull request Aug 25, 2020
ppisljar added a commit that referenced this pull request Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Drilldowns Embeddable panel Drilldowns 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

Development

Successfully merging this pull request may close these issues.

5 participants