-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Add pluggable panel action tests #20163
Changes from 2 commits
9f632bd
0cbc185
09585eb
a173d48
c9eec1b
5036e6e
07a4d3c
02304c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import React from 'react'; | ||
|
||
import { EventEmitter } from 'events'; | ||
import ReactDOM from 'react-dom'; | ||
|
||
// TODO: Remove once EUI has typing for EuiFlyout and EuiFlyoutBody | ||
declare module '@elastic/eui' { | ||
export const EuiFlyout: React.SFC<any>; | ||
} | ||
|
||
import { EuiFlyout } from '@elastic/eui'; | ||
|
||
let activeSession: FlyoutSession | null = null; | ||
|
||
const CONTAINER_ID = 'flyout-container'; | ||
|
||
function getOrCreateContainerElement() { | ||
let container = document.getElementById(CONTAINER_ID); | ||
if (!container) { | ||
container = document.createElement('div'); | ||
container.id = CONTAINER_ID; | ||
document.body.appendChild(container); | ||
} | ||
return container; | ||
} | ||
|
||
/** | ||
* An FlyoutSession describes the session of one opened flyout panel. It offers | ||
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. Typo: An -> A |
||
* methods to close the flyout panel again. If you open a flyout panel you should make | ||
* sure you call {@link FlyoutSession#close} when it should be closed. | ||
* Since a flyout could also be closed without calling this method (e.g. because | ||
* the user closes it), you must listen to the "closed" event on this instance. | ||
* It will be emitted whenever the flyout will be closed and you should throw | ||
* away your reference to this instance whenever you receive that event. | ||
* @extends EventEmitter | ||
*/ | ||
class FlyoutSession extends EventEmitter { | ||
/** | ||
* Binds the current flyout session to an Angular scope, meaning this flyout | ||
* session will be closed as soon as the Angular scope gets destroyed. | ||
* @param {object} scope - And angular scope object to bind to. | ||
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. Typo: And -> An (or The) |
||
*/ | ||
public bindToAngularScope(scope: ng.IScope): void { | ||
const removeWatch = scope.$on('$destroy', () => this.close()); | ||
this.on('closed', () => removeWatch()); | ||
} | ||
|
||
/** | ||
* Closes the opened flyout as long as it's stil the open one. | ||
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. Typo: stil -> still |
||
* If this is not the active session anymore, this method won't do anything. | ||
* If this session was still active and a flyout was closed, the 'closed' | ||
* event will be emitted on this FlyoutSession instance. | ||
*/ | ||
public close(): void { | ||
if (activeSession === this) { | ||
const container = document.getElementById(CONTAINER_ID); | ||
if (container) { | ||
ReactDOM.unmountComponentAtNode(container); | ||
this.emit('closed'); | ||
} | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Opens a flyout panel with the given component inside. You can use | ||
* {@link FlyoutSession#close} on the return value to close the flyout. | ||
* | ||
* @param flyoutChildren - Mounts the children inside a fly out panel | ||
* @return {FlyoutSession} The session instance for the opened flyout panel. | ||
*/ | ||
export function openFlyout( | ||
flyoutChildren: React.ReactNode, | ||
flyoutProps: { | ||
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. If possible, let's switch the order to first pass in the children. |
||
onClose?: () => void; | ||
'data-test-subj'?: string; | ||
} = {} | ||
): FlyoutSession { | ||
// If there is an active inspector session close it before opening a new one. | ||
if (activeSession) { | ||
activeSession.close(); | ||
} | ||
const container = getOrCreateContainerElement(); | ||
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 have a few thoughts about this. Should EuiFlyout itself just coordinate this internally, essentially acting like a singleton and ensuring only one flyout is ever visible at a time? It almost seems like that would be a sensible design. Alternatively, we could do that ourselves, and keep this inside the React lifecycle by just making it a plain React component. Something like this, maybe: // Something like this
let currentFlyout = null;
class SingletonFlyout {
componentWillMount() {
if (currentFlyout) {
currentFlyout.close();
}
currentFlyout = {
flyout: this,
close: this.props.onClose,
};
}
componentWillUnmount() {
if (currentFlyout && currentFlyout.flyout === this) {
currentFlyout = null;
}
}
render() {
return (
<EuiFlyout>
{this.props.children}
<EuiFlyout>
);
}
} The advantage there is that consumers can use it like any other React component. It'd be styled to have a fixed position, etc, but could show/ hide based on props / state changes without special cases or whatever. It'd be consumed like this: // in some stateful component's render...
render() {
return (
<div>
My usual component.
<button onClick={() => this.setState({ isFlying: true })}>
Fly, little bird!
</button>
{!this.state.isFlying ? null :
<SingletonFlyout onClose={() => this.setState({ isFlying: false })}>
<h1>Hi!</h1>
</SingletonFlyout>
}
</div>
);
} Or something. You could pass it an Angular scope as an optional param, too, if that's a case that needs handling. The advantage to either of those ^^^ approaches is that the lifecycle of the flyout is normal React stuff, no special DOM manipulation or possible leaking edge cases. If we keep the current solution (which is fine by me), it seems like it might be nice to optionally pass the container in, so that callers could pass a container that is managed by React / destroyed by React and not have to worry about cleanup or extraneous references. 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 prefer your first solutions - having flyout be a singleton. I can't think of any good situation where we should have two flyouts. I would prefer to leave it as is though in this PR, just for the sake of unblocking some things (Tim wants to add inspector tests, I want to extend the panel action tests, and we both want to generalize this approach to all plugins not just panels). I think we'll have to address the flyout changes soon, though, as what should happen if you open a panel action with a flyout while an inspector is open? |
||
const session = (activeSession = new FlyoutSession()); | ||
const onClose = () => { | ||
if (flyoutProps.onClose) { | ||
flyoutProps.onClose(); | ||
} | ||
session.close(); | ||
}; | ||
|
||
ReactDOM.render( | ||
<EuiFlyout {...flyoutProps} onClose={onClose}> | ||
{flyoutChildren} | ||
</EuiFlyout>, | ||
container | ||
); | ||
|
||
return session; | ||
} | ||
|
||
export { FlyoutSession }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
export * from './flyout_session'; |
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.
note: does this really give us any benefit comparing to just
ts-ignore
like we do for other cases?We talked about that inline
declare module
thing with @timroes the other day and obviously the ideal option would be to have types file in "eui" package, even very simple and incomplete types would be better than that, but only if eui owners would commit to keep these typings up to date. There is no need to write them all, just keep eye on existing types during review. Is it something we can do? Who is the owner of this package? @cjcenizal ?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.
Soon as we have a new eui package I can get rid of this. I went into eui and added the typings (elastic/eui#1001).
I think @chandlerprall is taking charge of the typescript efforts in EUI. I'm not sure what the plan is, but it seems a lot easier to upkeep if EUI used typescript instead of maintaining separate typings definitions. Perhaps that is the long term plan.
I'll wait for a new eui to submit this PR so I can get rid of this.
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.
Yep, Chandler owns the code side of EUI now. I agree that converting all of EUI to TS would make the most sense for ease-of-maintenance.
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.
Converting EUI to TS is the most likely solution. Either way we know we need to properly support TS consumers.