-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Remove react references from core OverlayService
apis
#48431
Remove react references from core OverlayService
apis
#48431
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
16fe10c
to
98d87e3
Compare
This comment has been minimized.
This comment has been minimized.
98d87e3
to
7667b28
Compare
This comment has been minimized.
This comment has been minimized.
7667b28
to
9d80529
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b17ec7f
to
f467e48
Compare
This comment has been minimized.
This comment has been minimized.
f467e48
to
c2b90c9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
92873f2
to
7e03f01
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
52309a2
to
3bdf701
Compare
This comment has been minimized.
This comment has been minimized.
retest |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
626ce2e
to
75ab490
Compare
…stic-overlay-service
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transform/ml change LGTM
…rom overlayService
@chandlerprall following our discussion on this PR, could you just confirm that the changes are ok for |
#49573 has been merged, PR now integrates these changes and is ready for review |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -29,7 +30,7 @@ import { | |||
EuiModalHeaderTitle, | |||
} from '@elastic/eui'; | |||
import { EuiSpacer } from '@elastic/eui'; | |||
import { FormattedMessage } from '@kbn/i18n/react'; | |||
import { FormattedMessage, I18nProvider } from '@kbn/i18n/react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point, this stateful import will probably go away. Can we pass this Provider down from the I18n service to future-proof this a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. However, the stateful import is also used in kibana_react
'stoMountPoint
kibana/src/plugins/kibana_react/public/util/react_mount.tsx
Lines 22 to 40 in e04adbe
import { I18nProvider } from '@kbn/i18n/react'; | |
import { MountPoint } from 'kibana/public'; | |
/** | |
* MountPoint converter for react nodes. | |
* | |
* @param node to get a mount point for | |
*/ | |
export const toMountPoint = (node: React.ReactNode): MountPoint => { | |
const mount = (element: HTMLElement) => { | |
ReactDOM.render(<I18nProvider>{node}</I18nProvider>, element); | |
return () => ReactDOM.unmountComponentAtNode(element); | |
}; | |
// only used for tests and snapshots serialization | |
if (process.env.NODE_ENV !== 'production') { | |
mount.__reactMount__ = node; | |
} | |
return mount; | |
}; |
Should this be a concern?
/** {@link OverlayFlyoutStart#open} */ | ||
openFlyout: OverlayFlyoutStart['open']; | ||
/** {@link OverlayModalStart#open} */ | ||
openModal: OverlayModalStart['open']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to make these APIs a bit more consistent with banners
in a separate change? (eg. overlays.flyout.open
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, but reverted it in e26120b following discussion with @rudolf starting at #48431 (comment) as there was no consensus. What should we do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion has addressed @elastic/kibana-design concerns
💚 Build Succeeded |
* move/rename overlay mount and unmount types from banner to parent module Signed-off-by: pgayvallet <[email protected]> * migrate openModal / modalService Signed-off-by: pgayvallet <[email protected]> fix I18nProvider import path Signed-off-by: pgayvallet <[email protected]> * updates core doc Signed-off-by: pgayvallet <[email protected]> update doc bis Signed-off-by: pgayvallet <[email protected]> * migrate openFlyout / flyout service Signed-off-by: pgayvallet <[email protected]> * remove CoreStart export from kibana-react Signed-off-by: pgayvallet <[email protected]> * adapt some calls to new signature Signed-off-by: pgayvallet <[email protected]> * adapt new calls Signed-off-by: pgayvallet <[email protected]> * adapt js call Signed-off-by: pgayvallet <[email protected]> * add flex layout on mountWrapper component to avoid losing scroll on overlays Signed-off-by: pgayvallet <[email protected]> * create proper flyout/modal services * update generated doc * update snapshot on data/query_bar * use ReactNode instead of ReactElement * rename mountForComponent to reactMount * change reactMount usages to toMountPoint * remove duplicate MountPoint type in overlays * remove duplicate mount utilities from overlays * allow to specify custom class name to MountWrapper * Allow to specialize MountPoint on HTMLElement subtypes * updates generated doc * undeprecates openFlyout/openModal & remove direct subservice access from overlayService * adapt toast api to get i18n context from service * use MountPoint instead of inline definition
So @pgayvallet @joshdover and I decided we'll try to apply the following principle in deciding how we expose API's: |
) * move/rename overlay mount and unmount types from banner to parent module Signed-off-by: pgayvallet <[email protected]> * migrate openModal / modalService Signed-off-by: pgayvallet <[email protected]> fix I18nProvider import path Signed-off-by: pgayvallet <[email protected]> * updates core doc Signed-off-by: pgayvallet <[email protected]> update doc bis Signed-off-by: pgayvallet <[email protected]> * migrate openFlyout / flyout service Signed-off-by: pgayvallet <[email protected]> * remove CoreStart export from kibana-react Signed-off-by: pgayvallet <[email protected]> * adapt some calls to new signature Signed-off-by: pgayvallet <[email protected]> * adapt new calls Signed-off-by: pgayvallet <[email protected]> * adapt js call Signed-off-by: pgayvallet <[email protected]> * add flex layout on mountWrapper component to avoid losing scroll on overlays Signed-off-by: pgayvallet <[email protected]> * create proper flyout/modal services * update generated doc * update snapshot on data/query_bar * use ReactNode instead of ReactElement * rename mountForComponent to reactMount * change reactMount usages to toMountPoint * remove duplicate MountPoint type in overlays * remove duplicate mount utilities from overlays * allow to specify custom class name to MountWrapper * Allow to specialize MountPoint on HTMLElement subtypes * updates generated doc * undeprecates openFlyout/openModal & remove direct subservice access from overlayService * adapt toast api to get i18n context from service * use MountPoint instead of inline definition
Summary
Refactor the
OverlayService
to use framework agnosticMountPoint
instead of react components.OverlayService.openModal
andOverlayService.openFlyout
to use the newMountPoint
interface introduced in Remove react references from coreNotifications
apis #49573 instead of direct react components usagemodals
andflyouts
toMountPoint
adapterFixes #37894
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriatelyDev Docs
The core
OverlayService
methods are now framework agnostic and no longer accept react components as input. Please usekibana_react
'stoMountPoint
utility to convert a react component to a mountPoint.For exemple:
Becomes: