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

Remove react references from core OverlayService apis #48431

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Oct 16, 2019

Summary

Refactor the OverlayService to use framework agnostic MountPoint instead of react components.

  • Modify OverlayService.openModal and OverlayService.openFlyout to use the new MountPoint interface introduced in Remove react references from core Notifications apis #49573 instead of direct react components usage
  • Create sub-services for modals and flyouts
  • Adapt existing calls to use the new toMountPoint adapter

Fixes #37894

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For 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 appropriately

Dev Docs

The core OverlayService methods are now framework agnostic and no longer accept react components as input. Please use kibana_react'stoMountPoint utility to convert a react component to a mountPoint.

For exemple:

core.overlays.openModal(<MyComponent/>)

Becomes:

core.overlays.openModal(toMountPoint(<MyComponent/>))

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

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch from 16fe10c to 98d87e3 Compare October 17, 2019 08:08
@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch from 98d87e3 to 7667b28 Compare October 17, 2019 10:50
@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch from 7667b28 to 9d80529 Compare October 17, 2019 13:04
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch from b17ec7f to f467e48 Compare October 18, 2019 14:15
@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch from f467e48 to c2b90c9 Compare October 23, 2019 14:18
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch from 92873f2 to 7e03f01 Compare October 24, 2019 14:05
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch from 52309a2 to 3bdf701 Compare October 24, 2019 18:31
@elasticmachine

This comment has been minimized.

@brianseeders
Copy link
Contributor

retest

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-37894-NP-agnostic-overlay-service branch 2 times, most recently from 626ce2e to 75ab490 Compare October 24, 2019 23:39
@pgayvallet pgayvallet requested a review from a team as a code owner November 18, 2019 08:13
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@walterra walterra left a 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

@pgayvallet
Copy link
Contributor Author

@chandlerprall following our discussion on this PR, could you just confirm that the changes are ok for kibana-design on the react wrapper approach (at least for now)?

@pgayvallet
Copy link
Contributor Author

#49573 has been merged, PR now integrates these changes and is ready for review

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@joshdover joshdover left a 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';
Copy link
Contributor

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?

Copy link
Contributor Author

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

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'];
Copy link
Contributor

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)

Copy link
Contributor Author

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?

Copy link
Contributor

@chandlerprall chandlerprall left a 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

@pgayvallet pgayvallet removed the review label Nov 18, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@pgayvallet pgayvallet merged commit 01bbbf4 into elastic:master Nov 19, 2019
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Nov 19, 2019
* move/rename overlay mount and unmount types from banner to parent module

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* migrate openModal / modalService

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

fix I18nProvider import path

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* updates core doc

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

update doc bis

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* migrate openFlyout / flyout service

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* remove CoreStart export from kibana-react

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* adapt some calls to new signature

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* adapt new calls

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* adapt js call

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* add flex layout on mountWrapper component to avoid losing scroll on overlays

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* 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
@rudolf
Copy link
Contributor

rudolf commented Nov 19, 2019

I don't think we can come up with any magic rules for how we group this and when it makes sense to introduce a third level like core.overlays.modal.* but as it stands having only one method in a namespace makes me feel like those methods are a bit lonely 😉

So @pgayvallet @joshdover and I decided we'll try to apply the following principle in deciding how we expose API's:
We will have a bias towards exposing API's as nested namespaces (e.g. core.overlays.modal.*) as this is more future-proof. But, we will only do so if we expect that there will be additional methods in that namespace in the near future. So as an example, if we expect core.overlay.modal.open to be the only modal method, we will rather collapse that into core.overlay.openModal.

pgayvallet added a commit that referenced this pull request Nov 19, 2019
)

* move/rename overlay mount and unmount types from banner to parent module

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* migrate openModal / modalService

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

fix I18nProvider import path

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* updates core doc

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

update doc bis

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* migrate openFlyout / flyout service

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* remove CoreStart export from kibana-react

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* adapt some calls to new signature

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* adapt new calls

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* adapt js call

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* add flex layout on mountWrapper component to avoid losing scroll on overlays

Signed-off-by: pgayvallet <pierre.gayvallet@elastic.co>

* 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
@rudolf rudolf mentioned this pull request Dec 6, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[new-platform] Refactor OverlayService to not require React to use
10 participants