-
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 Notifications
apis
#49573
Changes from 8 commits
688c5a2
678eb13
6b986ee
5464cb2
ee61086
bd036bc
a49de40
71f530b
b6f5d15
bb779fb
c41f2db
ce1d179
78d45af
7de4066
0f391b2
8e18eb0
ab81138
081f878
7fa9e5d
53d8da5
dad7ee6
b944611
5aede50
c869fea
912124c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<!-- Do not edit this file. It is automatically generated by API Documenter. --> | ||
|
||
[Home](./index.md) > [kibana-plugin-public](./kibana-plugin-public.md) > [MountPoint](./kibana-plugin-public.mountpoint.md) | ||
|
||
## MountPoint type | ||
|
||
A function that will mount the banner inside the provided element. | ||
|
||
<b>Signature:</b> | ||
|
||
```typescript | ||
export declare type MountPoint = (element: HTMLElement) => UnmountCallback; | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<!-- Do not edit this file. It is automatically generated by API Documenter. --> | ||
|
||
[Home](./index.md) > [kibana-plugin-public](./kibana-plugin-public.md) > [Toast](./kibana-plugin-public.toast.md) | ||
|
||
## Toast type | ||
|
||
<b>Signature:</b> | ||
|
||
```typescript | ||
export declare type Toast = ToastInputFields & { | ||
id: string; | ||
}; | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,5 +22,5 @@ add(toastOrTitle: ToastInput): Toast; | |
|
||
`Toast` | ||
|
||
a | ||
a [Toast](./kibana-plugin-public.toast.md) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,5 +22,5 @@ addDanger(toastOrTitle: ToastInput): Toast; | |
|
||
`Toast` | ||
|
||
a | ||
a [Toast](./kibana-plugin-public.toast.md) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,5 +23,5 @@ addError(error: Error, options: ErrorToastOptions): Toast; | |
|
||
`Toast` | ||
|
||
a | ||
a [Toast](./kibana-plugin-public.toast.md) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,5 +22,5 @@ addSuccess(toastOrTitle: ToastInput): Toast; | |
|
||
`Toast` | ||
|
||
a | ||
a [Toast](./kibana-plugin-public.toast.md) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,5 +22,5 @@ addWarning(toastOrTitle: ToastInput): Toast; | |
|
||
`Toast` | ||
|
||
a | ||
a [Toast](./kibana-plugin-public.toast.md) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<!-- Do not edit this file. It is automatically generated by API Documenter. --> | ||
|
||
[Home](./index.md) > [kibana-plugin-public](./kibana-plugin-public.md) > [UnmountCallback](./kibana-plugin-public.unmountcallback.md) | ||
|
||
## UnmountCallback type | ||
|
||
A function that will unmount the element previously mounted by the associated [MountPoint](./kibana-plugin-public.mountpoint.md) | ||
|
||
<b>Signature:</b> | ||
|
||
```typescript | ||
export declare type UnmountCallback = () => void; | ||
``` |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,20 +17,38 @@ | |
* under the License. | ||
*/ | ||
|
||
import { EuiGlobalToastList, EuiGlobalToastListToast as Toast } from '@elastic/eui'; | ||
|
||
import { EuiGlobalToastList, EuiGlobalToastListToast as EuiToast } from '@elastic/eui'; | ||
import React from 'react'; | ||
import * as Rx from 'rxjs'; | ||
import { isString, omit } from 'lodash'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with other commenters about removing reliance on |
||
|
||
import { MountWrapper } from '../../utils'; | ||
import { Toast } from './toasts_api'; | ||
|
||
interface Props { | ||
toasts$: Rx.Observable<Toast[]>; | ||
dismissToast: (t: Toast) => void; | ||
dismissToast: (toastId: string) => void; | ||
} | ||
|
||
interface State { | ||
toasts: Toast[]; | ||
} | ||
|
||
const convertToEui = (toast: Toast): EuiToast => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be simplified by using spread, which should help eliminate the inline function and need for const convertToEui = (toast: Toast): EuiToast => ({
...toast,
title: typeof toast.title === 'function' ? <MountWrapper mount={toast.title} /> : value,
text: typeof toast.text === 'function' ? <MountWrapper mount={toast.text} /> : value,
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a subtle difference here: if EDIT: err, you will generate an |
||
const wrap = (value: Toast['title'] | Toast['text']) => | ||
isString(value) || value === undefined ? value : <MountWrapper mount={value} />; | ||
const euiToast: EuiToast = { | ||
...omit(toast, ['title', 'text']), | ||
}; | ||
if (toast.title !== undefined) { | ||
euiToast.title = wrap(toast.title); | ||
} | ||
if (toast.text !== undefined) { | ||
euiToast.text = wrap(toast.text); | ||
} | ||
return euiToast; | ||
}; | ||
|
||
export class GlobalToastList extends React.Component<Props, State> { | ||
public state: State = { | ||
toasts: [], | ||
|
@@ -54,8 +72,8 @@ export class GlobalToastList extends React.Component<Props, State> { | |
return ( | ||
<EuiGlobalToastList | ||
data-test-subj="globalToastList" | ||
toasts={this.state.toasts} | ||
dismissToast={this.props.dismissToast} | ||
toasts={this.state.toasts.map(convertToEui)} | ||
dismissToast={toast => this.props.dismissToast(toast.id)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we now got our own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can shorten with a destructure: dismissToast={({ id }) => this.props.dismissToast(id)} |
||
/** | ||
* This prop is overriden by the individual toasts that are added. | ||
* Use `Infinity` here so that it's obvious a timeout hasn't been | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,11 +17,13 @@ | |
* under the License. | ||
*/ | ||
|
||
import { EuiGlobalToastListToast as Toast } from '@elastic/eui'; | ||
import { EuiGlobalToastListToast as EuiToast } from '@elastic/eui'; | ||
import React from 'react'; | ||
import * as Rx from 'rxjs'; | ||
import { isString } from 'lodash'; | ||
|
||
import { ErrorToast } from './error_toast'; | ||
import { MountPoint, mountReact } from '../../utils'; | ||
import { UiSettingsClientContract } from '../../ui_settings'; | ||
import { OverlayStart } from '../../overlays'; | ||
|
||
|
@@ -33,13 +35,20 @@ import { OverlayStart } from '../../overlays'; | |
* | ||
* @public | ||
*/ | ||
export type ToastInputFields = Pick<Toast, Exclude<keyof Toast, 'id'>>; | ||
export type ToastInputFields = Pick<EuiToast, Exclude<keyof EuiToast, 'id' | 'text' | 'title'>> & { | ||
title?: string | MountPoint; | ||
text?: string | MountPoint; | ||
}; | ||
|
||
export type Toast = ToastInputFields & { | ||
id: string; | ||
}; | ||
|
||
/** | ||
* Inputs for {@link IToasts} APIs. | ||
* @public | ||
*/ | ||
export type ToastInput = string | ToastInputFields | Promise<ToastInputFields>; | ||
export type ToastInput = string | ToastInputFields; | ||
Comment on lines
-42
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
/** | ||
* Options available for {@link IToasts} APIs. | ||
|
@@ -59,13 +68,12 @@ export interface ErrorToastOptions { | |
toastMessage?: string; | ||
} | ||
|
||
const normalizeToast = (toastOrTitle: ToastInput) => { | ||
const normalizeToast = (toastOrTitle: ToastInput): ToastInputFields => { | ||
if (typeof toastOrTitle === 'string') { | ||
return { | ||
title: toastOrTitle, | ||
}; | ||
} | ||
|
||
return toastOrTitle; | ||
}; | ||
|
||
|
@@ -123,11 +131,12 @@ export class ToastsApi implements IToasts { | |
|
||
/** | ||
* Removes a toast from the current array of toasts if present. | ||
* @param toast - a {@link Toast} returned by {@link ToastApi.add} | ||
* @param toastOrId - a {@link Toast} returned by {@link ToastsApi.add} or it's id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/it's/its/ |
||
*/ | ||
public remove(toast: Toast) { | ||
public remove(toastOrId: Toast | string) { | ||
const toRemove = isString(toastOrId) ? toastOrId : toastOrId.id; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be inlined into the const listWithoutToast = list.filter(t => t.id !== (typeof toastOrId === 'string' ? toastOrId : toastOrId.id)); |
||
const list = this.toasts$.getValue(); | ||
const listWithoutToast = list.filter(t => t !== toast); | ||
const listWithoutToast = list.filter(t => t.id !== toRemove); | ||
if (listWithoutToast.length !== list.length) { | ||
this.toasts$.next(listWithoutToast); | ||
} | ||
|
@@ -191,7 +200,7 @@ export class ToastsApi implements IToasts { | |
iconType: 'alert', | ||
title: options.title, | ||
toastLifeTimeMs: this.uiSettings.get('notifications:lifetime:error'), | ||
text: ( | ||
text: mountReact( | ||
<ErrorToast | ||
openModal={this.openModal.bind(this)} | ||
error={error} | ||
|
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.
That seems to be the first time we have types in core that are used in different services (here both
Notification
andOverlay
), so I did put them inutils
. However not sure that's really the correct place. Maybe we want to create acore/public/common
orcore/public/shared
? WDYT ?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 think this is something we can expose from the Application Service, but still export them from the top-level (e.g.
import { Mount } from 'src/core/public';
. Right now the app service has interfaces forAppMountParameters
andApp
which have similar properties:We could have a generic
Mount
handler exported from there which can be inherited as well to solve these:Thoughts? cc: @joshdover
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.
Just to be sure I understand this correctly:
would become
Is that correct ?
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.
Also, as these type/interfaces are going to be imported from other
core/public
modules, is it the correct good practice to put them incore/public/application
and import it from here in other modules (likecore/public/notifications
), or should we put it in a more 'shared' module ?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'm actually of the opinion we should hold off on doing this until we remove context in #49691. As it is right now, I don't think the examples above are actually quite compatible with the existing AppMount interface. No sense in breaking that interface twice, just to share types.
In terms of where to put the shared types now, I think a
src/core/public/types
file makes sense. We have a similar file on the server-side.