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

[dashboard] Migrate Dashboard internal components to DashboardApi #193220

Merged
merged 13 commits into from
Sep 23, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Sep 17, 2024

PR replaces useDashboardContainer with useDashboardApi. useDashboardApi returns DashboardApi instead of DashboardContainer.

After this PR, all react context's in dashboard return DashboardApi and thus all components are now prepared for the migration from DashboardContainer to DashboardApi.

@nreese
Copy link
Contributor Author

nreese commented Sep 18, 2024

/ci

@nreese nreese changed the title remove useDashboardContainer [dashboard] Migrate Dashboard internal components to DashboardApi Sep 18, 2024
@nreese
Copy link
Contributor Author

nreese commented Sep 18, 2024

/ci

@nreese nreese marked this pull request as ready for review September 18, 2024 20:27
@nreese nreese requested a review from a team as a code owner September 18, 2024 20:27
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes v9.0.0 project:embeddableRebuild v8.16.0 labels Sep 18, 2024
@elasticmachine
Copy link
Contributor

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

@Heenawter Heenawter self-requested a review September 18, 2024 20:31
Copy link
Contributor

@Heenawter Heenawter 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 - I believe isEmbeddedExternally is component state and not explicit input. This is technically a bug since it cannot currently be set, so after that is fixed + a quick sanity test locally, I can approve :)

Comment on lines +55 to +61
const { originatingPath, originatingApp } = useMemo(() => {
const appContext = dashboardApi.getAppContext();
return {
originatingApp: appContext?.currentAppId,
originatingPath: appContext?.getCurrentPath?.() ?? '',
};
}, [dashboardApi]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

@nreese
Copy link
Contributor Author

nreese commented Sep 19, 2024

Code review - I believe isEmbeddedExternally is component state and not explicit input. This is technically a bug since it cannot currently be set, so after that is fixed + a quick sanity test locally, I can approve :)

Thanks for catching this. It is indeed part of component state. I have committed the suggested changes that resolve this.

@nreese nreese added the backport:version Backport to applied version labels label Sep 19, 2024
Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nreese
Copy link
Contributor Author

nreese commented Sep 20, 2024

@elasticmachine merge upstream

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

See #193611 👍

@nreese
Copy link
Contributor Author

nreese commented Sep 20, 2024

See #193611

Resolved with bba739d

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the controls issue 👍 Tested, and it works great now

@nreese
Copy link
Contributor Author

nreese commented Sep 23, 2024

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

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

id before after diff
dashboard 481.6KB 490.6KB +9.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
dashboard 13 15 +2

Page load bundle

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

id before after diff
dashboard 26.9KB 26.9KB -1.0B

History

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

@nreese nreese merged commit 92da176 into elastic:main Sep 23, 2024
21 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 23, 2024
…astic#193220)

PR replaces `useDashboardContainer` with `useDashboardApi`.
`useDashboardApi` returns `DashboardApi` instead of
`DashboardContainer`.

After this PR, all react context's in dashboard return `DashboardApi`
and thus all components are now prepared for the migration from
DashboardContainer to DashboardApi.

---------

Co-authored-by: Hannah Mudge <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 92da176)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 23, 2024
…pi (#193220) (#193739)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[dashboard] Migrate Dashboard internal components to DashboardApi
(#193220)](#193220)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-23T15:21:37Z","message":"[dashboard]
Migrate Dashboard internal components to DashboardApi (#193220)\n\nPR
replaces `useDashboardContainer` with
`useDashboardApi`.\r\n`useDashboardApi` returns `DashboardApi` instead
of\r\n`DashboardContainer`.\r\n\r\nAfter this PR, all react context's in
dashboard return `DashboardApi`\r\nand thus all components are now
prepared for the migration from\r\nDashboardContainer to
DashboardApi.\r\n\r\n---------\r\n\r\nCo-authored-by: Hannah Mudge
<[email protected]>\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"92da1767f3bab00f5b7abca16daec06e2314302b","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","release_note:skip","v9.0.0","project:embeddableRebuild","v8.16.0","backport:version"],"title":"[dashboard]
Migrate Dashboard internal components to
DashboardApi","number":193220,"url":"https://github.com/elastic/kibana/pull/193220","mergeCommit":{"message":"[dashboard]
Migrate Dashboard internal components to DashboardApi (#193220)\n\nPR
replaces `useDashboardContainer` with
`useDashboardApi`.\r\n`useDashboardApi` returns `DashboardApi` instead
of\r\n`DashboardContainer`.\r\n\r\nAfter this PR, all react context's in
dashboard return `DashboardApi`\r\nand thus all components are now
prepared for the migration from\r\nDashboardContainer to
DashboardApi.\r\n\r\n---------\r\n\r\nCo-authored-by: Hannah Mudge
<[email protected]>\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"92da1767f3bab00f5b7abca16daec06e2314302b"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193220","number":193220,"mergeCommit":{"message":"[dashboard]
Migrate Dashboard internal components to DashboardApi (#193220)\n\nPR
replaces `useDashboardContainer` with
`useDashboardApi`.\r\n`useDashboardApi` returns `DashboardApi` instead
of\r\n`DashboardContainer`.\r\n\r\nAfter this PR, all react context's in
dashboard return `DashboardApi`\r\nand thus all components are now
prepared for the migration from\r\nDashboardContainer to
DashboardApi.\r\n\r\n---------\r\n\r\nCo-authored-by: Hannah Mudge
<[email protected]>\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"92da1767f3bab00f5b7abca16daec06e2314302b"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Nathan Reese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants