-
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
[new-platform] Refactor OverlayService to not require React to use #37894
Comments
Pinging @elastic/kibana-platform |
I was suggesting here this interface could be standartized, for example, called export interface OverlayStart {
openFlyout: MountPoint;
openModal: MountPoint;
} and for the export interface ApplicationSetup {
renderIcon: MountPoint;
renderApp: MountPoint;
} |
We want something framework agnostic, but React is render-target agnostic.
If we take an example ( const modal = npStart.core.overlays.openModal(
<ShardFailureModal
request={request}
response={response}
title={title}
onClose={() => modal.close()}
/>,
{
className: 'shardFailureModal',
}
); What would be the conversion, does something like works ? let modal = null;
const modalContent = (<ShardFailureModal
request={request}
response={response}
title={title}
onClose={() => modal.close()}
/>)
const wrapper = document.createElement('div');
ReactDOM.render(modalContent, wrapper);
modal = npStart.core.overlays.openModal(wrapper, {}); Maybe the responsibilities should be reversed? I'm thinking of something like this: modal = npStart.core.overlays.openModal(
(modalRoot: HTMLElement) => ReactDOM.render(modalContent, modalRoot), {}
); Or am I missing an easier way to actually get a ref from an unmounted react element ? |
As @cjcenizal explains here: #36425 (comment), the underlying EUI components we are using are React components. That's why currently, using this framework is the easiest way to interact with modals, flyover and toasts. We can probably (and need to) add an abstraction level, but even in that case, I think keeping the possibility to actually use 'native' React element will be the best way to not impact the developer experience (asking to migrate to React while only exposing agnostic UI APIs seems questionnable to me). If we see my previous exemple, there is a difference between const modal = npStart.core.overlays.openModal(<MyComponent/>); and const modalContent = <MyComponent/>;
npStart.core.overlays.openModal(
(modalRoot: HTMLElement) => ReactDOM.render(modalContent, modalRoot), {})
) More importantly, as long as we keep using EUI components as our implementation, any non-react content passed to any modal, toast or flyover will need to be converted back into react world, and the only option for this is However, I agree that we still needs to allow developers to use any other framework that React, and we need to extend the current apis. That's why I'd like to propose something slightly different than your interface @joshdover: // naming open to proposal, not really fond of this
type ManualRender = (parent: HTMLElement) => void;
type DomContent = React.ReactNode | HTMLElement | ManualRender | string; // do we want to allow stringified html ?
export interface OverlayStart {
/** {@link OverlayBannersStart} */
banners: OverlayBannersStart;
openFlyout: (
flyoutChildren: DomContent,
flyoutProps?: {
closeButtonAriaLabel?: string;
'data-test-subj'?: string;
}
) => OverlayRef;
openModal: (
modalChildren: DomContent,
modalProps?: {
className?: string;
closeButtonAriaLabel?: string;
'data-test-subj'?: string;
}
) => OverlayRef;
} That way we keep React as a first-class citizen of the NP, while staying agnostic by allowing other way to use these services without react. Regarding the name |
We talked offline, but for now, we're going to stick with the raw DOM element approach to fulfill these 3 goals:
How we improve the ergonomics here is something we should discuss, but whatever we adopt should be something we support on all Core APIs. |
To keep the framework agnostic approach, we will then re-use the types already defined in the banner service (with proper renaming to something more generic. @streamich suggestion of type Unmount = () => void;
type MountPoint = (element: HTMLElement) => Unmount; not sure how to properly name the |
|
Steps to view problem * install sample data set * Open lens visualization * Open inspector. Notice console errors <img width="300" alt="Screen Shot 2023-05-05 at 11 03 25 AM" src="https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png"> elastic/eui#6566 removed `closeButtonAriaLabel` prop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI 75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into `EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom and causing error. `OverlayFlyoutOpenOptions` type added by #37894. I replaced `OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear what props are accepted and provide stronger typing that stays in sync with EUI typings --------- Co-authored-by: kibanamachine <[email protected]>
Steps to view problem * install sample data set * Open lens visualization * Open inspector. Notice console errors <img width="300" alt="Screen Shot 2023-05-05 at 11 03 25 AM" src="https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png"> elastic/eui#6566 removed `closeButtonAriaLabel` prop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI 75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into `EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom and causing error. `OverlayFlyoutOpenOptions` type added by elastic#37894. I replaced `OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear what props are accepted and provide stronger typing that stays in sync with EUI typings --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit b803ba9)
# Backport This will backport the following commits from `main` to `8.8`: - [fix console errors in inspector (#156894)](#156894) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Nathan Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-15T15:02:27Z","message":"fix console errors in inspector (#156894)\n\nSteps to view problem\r\n* install sample data set\r\n* Open lens visualization\r\n* Open inspector. Notice console errors\r\n<img width=\"300\" alt=\"Screen Shot 2023-05-05 at 11 03 25 AM\"\r\nsrc=\"https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png\">\r\n\r\nhttps://github.com/elastic/eui/pull/6566 removed `closeButtonAriaLabel`\r\nprop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI\r\n75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into\r\n`EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom\r\nand causing error.\r\n\r\n`OverlayFlyoutOpenOptions` type added by\r\nhttps://github.com//issues/37894. I replaced\r\n`OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear\r\nwhat props are accepted and provide stronger typing that stays in sync\r\nwith EUI typings\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"b803ba9d7b69250d8bfb0567919128f954c1e935","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Inspector","Team:Presentation","release_note:skip","v8.8.0","v8.9.0"],"number":156894,"url":"https://github.com/elastic/kibana/pull/156894","mergeCommit":{"message":"fix console errors in inspector (#156894)\n\nSteps to view problem\r\n* install sample data set\r\n* Open lens visualization\r\n* Open inspector. Notice console errors\r\n<img width=\"300\" alt=\"Screen Shot 2023-05-05 at 11 03 25 AM\"\r\nsrc=\"https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png\">\r\n\r\nhttps://github.com/elastic/eui/pull/6566 removed `closeButtonAriaLabel`\r\nprop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI\r\n75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into\r\n`EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom\r\nand causing error.\r\n\r\n`OverlayFlyoutOpenOptions` type added by\r\nhttps://github.com//issues/37894. I replaced\r\n`OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear\r\nwhat props are accepted and provide stronger typing that stays in sync\r\nwith EUI typings\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"b803ba9d7b69250d8bfb0567919128f954c1e935"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156894","number":156894,"mergeCommit":{"message":"fix console errors in inspector (#156894)\n\nSteps to view problem\r\n* install sample data set\r\n* Open lens visualization\r\n* Open inspector. Notice console errors\r\n<img width=\"300\" alt=\"Screen Shot 2023-05-05 at 11 03 25 AM\"\r\nsrc=\"https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png\">\r\n\r\nhttps://github.com/elastic/eui/pull/6566 removed `closeButtonAriaLabel`\r\nprop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI\r\n75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into\r\n`EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom\r\nand causing error.\r\n\r\n`OverlayFlyoutOpenOptions` type added by\r\nhttps://github.com//issues/37894. I replaced\r\n`OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear\r\nwhat props are accepted and provide stronger typing that stays in sync\r\nwith EUI typings\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"b803ba9d7b69250d8bfb0567919128f954c1e935"}}]}] BACKPORT--> Co-authored-by: Nathan Reese <[email protected]>
Steps to view problem * install sample data set * Open lens visualization * Open inspector. Notice console errors <img width="300" alt="Screen Shot 2023-05-05 at 11 03 25 AM" src="https://user-images.githubusercontent.com/373691/236521366-d8fb9302-e93b-4047-a0bf-d7c09dcc3ffb.png"> elastic/eui#6566 removed `closeButtonAriaLabel` prop from [EuiFlyout](https://elastic.github.io/eui/#/layout/flyout) EUI 75.0.0 (Effecting 8.8 and 8.9). FlyoutService spreads options into `EuiFlyout`, resulting in `closeButtonAriaLabel` getting added to dom and causing error. `OverlayFlyoutOpenOptions` type added by #37894. I replaced `OverlayFlyoutOpenOptions` with `EuiFlyoutProps` to make it more clear what props are accepted and provide stronger typing that stays in sync with EUI typings --------- Co-authored-by: kibanamachine <[email protected]>
Currently, the
OverlayService
exposes a React interface from Core. All APIs in Core should be Vanilla JavaScript and not dependent on any specific rendering framework, even if they use one under the hood.We need to fix this inconsistency, here's my proposed interface.
The text was updated successfully, but these errors were encountered: