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

[AppEx-SharedUX] Remove toMountPoint parameter from TableListViewKibanaProvider #182030

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Apr 29, 2024

Summary

The toMountPoint module from @kbn/kibana-react-plugin/public is deprecated. Many areas of code need to include it and provide it as a required prop of TableListViewKibanaProvider. This PR resolves that: it shouldn't need to be a required prop anywhere, since it is a stateless component and can be included as-needed.

This PR also updates the inclusion of toMountPoint to use the non-deprecated source, which is @kbn/react-kibana-mount. As a side effect of that, some consumers need to update the core property being passed, to ensure it includes the analytics, i18n and theme services.

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan force-pushed the sharedux/clean-up-table-list-view-kibana-provider-props branch from 3b592ad to 0b42eb1 Compare April 29, 2024 20:21
@tsullivan tsullivan force-pushed the sharedux/clean-up-table-list-view-kibana-provider-props branch from eff1e14 to b930894 Compare April 29, 2024 20:57
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan marked this pull request as ready for review April 29, 2024 22:53
@tsullivan tsullivan requested review from a team as code owners April 29, 2024 22:53
@tsullivan tsullivan changed the title [SharedUX] use toMountPoint package internally [AppEx-SharedUX] Remove toMountPoint parameter from TableListViewKibanaProvider Apr 29, 2024
@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Apr 29, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

Dashboard and maps changes LGTM 👍

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

Vis changes LGTM 👌🏼

@Dosant Dosant self-requested a review April 30, 2024 14:42
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

lgtm, sorry for the merge conflict

@@ -26,6 +26,8 @@
"@kbn/shared-ux-link-redirect-app",
"@kbn/test-jest-helpers",
"@kbn/content-management-table-list-view-common",
"@kbn/core",
Copy link
Contributor

Choose a reason for hiding this comment

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

probably nit: I prefer to depend on specific packages types in packages instead of depending on the whole core

Dosant and others added 3 commits April 30, 2024 16:57
…up-table-list-view-kibana-provider-props

# Conflicts:
#	packages/content-management/table_list_view_table/src/services.tsx
#	packages/content-management/table_list_view_table/tsconfig.json
#	src/plugins/dashboard/public/dashboard_listing/dashboard_listing_table.tsx
@tsullivan tsullivan enabled auto-merge (squash) April 30, 2024 16:59
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 458 459 +1
eventAnnotationListing 520 534 +14
filesManagement 150 164 +14
total +29

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
@kbn/content-management-table-list-view-table 34 31 -3

Async chunks

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

id before after diff
dashboard 407.1KB 407.2KB +70.0B
eventAnnotationListing 208.4KB 209.4KB +1.1KB
filesManagement 100.3KB 101.4KB +1.1KB
graph 397.3KB 397.2KB -94.0B
maps 3.0MB 3.0MB +1.1KB
visualizations 283.4KB 283.3KB -66.0B
total +3.1KB

Page load bundle

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

id before after diff
eventAnnotationListing 10.7KB 10.7KB -21.0B
filesManagement 3.7KB 3.8KB +68.0B
maps 50.5KB 50.6KB +68.0B
total +115.0B
Unknown metric groups

API count

id before after diff
@kbn/content-management-table-list-view-table 51 47 -4

References to deprecated APIs

id before after diff
dashboard 28 24 -4
eventAnnotationListing 2 0 -2
filesManagement 6 4 -2
graph 63 61 -2
maps 33 31 -2
visualizations 62 60 -2
total -14

History

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

@tsullivan tsullivan merged commit d059886 into elastic:main Apr 30, 2024
19 checks passed
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels Apr 30, 2024
@tsullivan tsullivan deleted the sharedux/clean-up-table-list-view-kibana-provider-props branch April 30, 2024 21:41
yuliacech pushed a commit to yuliacech/kibana that referenced this pull request May 3, 2024
…naProvider (elastic#182030)

## Summary

The `toMountPoint` module from `@kbn/kibana-react-plugin/public` is
deprecated. Many areas of code need to include it and provide it as a
required prop of `TableListViewKibanaProvider`. This PR resolves that:
it shouldn't need to be a required prop anywhere, since it is a
stateless component and can be included as-needed.

This PR also updates the inclusion of `toMountPoint` to use the
non-deprecated source, which is `@kbn/react-kibana-mount`. As a side
effect of that, some consumers need to update the `core` property being
passed, to ensure it includes the `analytics`, `i18n` and `theme`
services.

### Checklist

Delete any items that are not applicable to this PR.

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Anton Dosov <anton.dosov@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants