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

Refactor NotificationsService to not accept React components #36425

Closed
streamich opened this issue May 10, 2019 · 6 comments · Fixed by #49573
Closed

Refactor NotificationsService to not accept React components #36425

streamich opened this issue May 10, 2019 · 6 comments · Fixed by #49573
Assignees
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@streamich
Copy link
Contributor

Kibana version: 8.0.0

Describe the bug: New platform accepts React components for toast notifications.

Steps to reproduce:

image

Expected behavior: New platform should be library agnostic.

@streamich streamich added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels May 10, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@streamich
Copy link
Contributor Author

Also flyout and modal:

image

@streamich streamich changed the title New platform toasts NP UI Integration May 31, 2019
@streamich
Copy link
Contributor Author

Should we discuss creating a common interface that will be used for all plugin integration points with platform UI?

Would be also great to use that for app icon instead of implementing euiIconType and icon.

@joshdover joshdover changed the title NP UI Integration Refactor NotificationsService to not accept React components Jun 26, 2019
@cjcenizal
Copy link
Contributor

cjcenizal commented Jul 23, 2019

New platform should be library agnostic.

I'd like to suggest we continue to support the existing interface, but enhance it (without introducing breaking changes) to enable a similar level of control over the toast for consumers that use other frameworks or no framework at all. Here's my line of thought...

  1. EUI is Elastic's solution for building the UI layer for our apps, and it's React, so realistically React is Elastic's de facto tool for building user interfaces (Kibana included). I'm not arguing this is right or ideal, but I think it's an accurate description of reality.
  2. It's a critical goal that the New Platform enable plugin developers to build plugins using whatever tools, libraries, and frameworks are best for them -- this is what I understand to be the essence of the term "library agnostic". To me, "library agnostic" doesn't mean the NP can't consider other libraries whatsoever -- it just means more of a "do no harm" approach, where it won't force plugin developers to use specific tools or libraries.
  3. I think the right balance is to develop interfaces that actively support 1 while being careful to respect 2. In other words, I think we should make it easy to write React plugins (since that's what all of Elastic's plugins will be written in), but possible to write non-React plugins (to maximize our inclusion of the community).

Using toast notifications as an example, I'd suggest adding additional config parameters to support arbitrary rendering to the toast's title and text areas. This way the consumer can customize those things just like React code can, but without needing React. Something like this:

toasts.add({
  onAdd: ({ title, text}) => {
    // These are just DOM nodes so you can do anything with them.
    text.appendElement(toastBody);
  }),
  onRemove: ({ title, text }) => {
    // We'll probably need to support cleanup logic as well.
  }),
});

@pgayvallet
Copy link
Contributor

This and #37894 feels like the same discussion to me. Please see #37894 (comment)

@pgayvallet
Copy link
Contributor

The current Toast type is the following:

type toast = {
  text?: ReactChild;
  toastLifeTimeMs?: number;
  title?: ReactNode;
  color?: ToastColor;
  iconType?: IconType;
  onClose?: () => void;
}

with nested types being

type ReactChild = ReactElement | ReactText;
type IconType = EuiIconType | string | ReactElement;
type ReactNode = ReactChild | ReactFragment | ReactPortal | boolean | null | undefined;

The text and title are going to change to string | MountPoint, with MountPoint being

/**
 * A function that will mount the banner inside the provided element.
 * @param element the container element to render into
 * @returns a {@link UnmountCallback} that unmount the element on call.
 *
 * @public
 */
export type MountPoint = (element: HTMLElement) => UnmountCallback;

/**
 * A function that will unmount the element previously mounted by
 * the associated {@link MountPoint}
 *
 * @public
 */
export type UnmountCallback = () => void;

(same as in #48431)

However, not sure what to do about iconType. It's currently very strongly coupled with both React (ReactElement) and EUI (EuiIconType). I think we should remove the ReactElement option, however, it's probably very hard to abstract from EuiIconType without duplicating their list of icon, so I guess we will still be dependent on EUI on this part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants