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] remove EmbeddableInput type #211949

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[embeddable] remove EmbeddableInput type #211949

wants to merge 10 commits into from
+116 −220

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Feb 20, 2025 •
edited
Loading

EmbeddableInput type is part of the legacy embeddable system. The legacy embeddable system is being removed and as such, the EmbeddableInput type is being removed.

@nreese
Copy link
Contributor Author

nreese commented Mar 3, 2025

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Mar 3, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Mar 4, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Mar 4, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Mar 4, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Mar 4, 2025

/ci

@nreese nreese marked this pull request as ready for review March 4, 2025 18:58
@nreese nreese requested review from a team as code owners March 4, 2025 18:58
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes project:embeddableRebuild backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Mar 4, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

ThomThomson
ThomThomson approved these changes Mar 4, 2025
View reviewed changes
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.

Type removal LGTM! NIce!

Left one nit

...m/plugins/shared/dashboard/server/dashboard_saved_object/migrations/migrate_hidden_titles.ts
newPanels.push(
convertPanelStateToSavedDashboardPanel(panel.panelIndex, {
...originalPanelState,
explicitInput: {
...originalPanelState.explicitInput,
...(originalPanelState.explicitInput.title === '' &&
!originalPanelState.explicitInput.hidePanelTitles
...((originalPanelState.explicitInput as { title?: string }).title === '' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can use one of the type guards like stateHasTitles from src/platform/packages/shared/presentation/presentation_publishing/interfaces/titles/title_manager.ts instead of just casting with as here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

presentation_publishing is shared-browser plugin. Is it worth making this shared-common plugin so it can be used in server?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, likely not worth the change without further discussion

@botelastic botelastic bot added Feature:Embedding Embedding content via iFrame Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Mar 4, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@nreese
Copy link
Contributor Author

nreese commented Mar 4, 2025

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
embeddable 83 82 -1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
dashboard 89 88 -1
embeddable 136 131 -5
total -6

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 5.3MB 5.3MB -27.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
embeddable 20.9KB 20.5KB -372.0B
Unknown metric groups

API count

id before after diff
dashboard 93 92 -1
embeddable 163 158 -5
total -6

ESLint disabled line counts

id before after diff
ml 561 560 -1

Total ESLint disabled count

id before after diff
ml 564 563 -1

History

dej611
dej611 approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review only as only types are touched in @elastic/kibana-visualizations owned files. 👍

jennypavlova
jennypavlova approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

APM changes LGTM

jgowdyelastic
jgowdyelastic approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson approved these changes

@dej611 dej611 dej611 approved these changes

@jennypavlova jennypavlova jennypavlova approved these changes

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

Assignees
No one assigned
Labels
backport:version Backport to applied version labels Feature:Embedding Embedding content via iFrame project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.19.0 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nreese @elasticmachine @dej611 @jennypavlova @ThomThomson @jgowdyelastic @kibanamachine