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.

Sign up for GitHub

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

Jump to bottom

Add pluggable panel action tests #20163

Merged
merged 8 commits into from
Jul 28, 2018
Merged

Add pluggable panel action tests #20163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9f632bd
Add pluggable panel action tests
stacey-gammon Jun 22, 2018
0cbc185
address code review comments
stacey-gammon Jul 12, 2018
09585eb
Merge branch 'master' of https://github.com/elastic/kibana into 2018-…
stacey-gammon Jul 17, 2018
a173d48
update inspector snapshot
stacey-gammon Jul 17, 2018
c9eec1b
Merge branch 'master' of https://github.com/elastic/kibana into 2018-…
stacey-gammon Jul 24, 2018
5036e6e
Merge branch 'master' of https://github.com/elastic/kibana into 2018-…
stacey-gammon Jul 26, 2018
07a4d3c
remove temp declared ts module now that eui has EuiFlyout typings
stacey-gammon Jul 26, 2018
02304c5
address code comments
stacey-gammon Jul 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions scripts/functional_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,5 @@ require('../src/setup_node_env');
require('../packages/kbn-test').runTestsCli([
require.resolve('../test/functional/config.js'),
require.resolve('../test/api_integration/config.js'),
require.resolve('../test/panel_actions/config.js'),
]);
115 changes: 115 additions & 0 deletions src/ui/public/flyout/flyout_session.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/*
* 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 { EuiFlyout } from '@elastic/eui';
import { EventEmitter } from 'events';
import ReactDOM from 'react-dom';

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;
}

/**
* A FlyoutSession describes the session of one opened flyout panel. It offers
* 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 - An angular scope object to bind to.
*/
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 still the open one.
* 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: {
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();
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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 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 };
20 changes: 20 additions & 0 deletions src/ui/public/flyout/index.ts
View file
Open in desktop
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';
80 changes: 6 additions & 74 deletions src/ui/public/inspector/inspector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,67 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/

import { EventEmitter } from 'events';
import React from 'react';
import ReactDOM from 'react-dom';

import { FlyoutSession, openFlyout } from 'ui/flyout';
import { Adapters } from './types';
import { InspectorPanel } from './ui/inspector_panel';
import { viewRegistry } from './view_registry';

let activeSession: InspectorSession | null = null;

const CONTAINER_ID = 'inspector-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 InspectorSession describes the session of one opened inspector. It offers
* methods to close the inspector again. If you open an inspector you should make
* sure you call {@link InspectorSession#close} when it should be closed.
* Since an inspector 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 inspector will be closed and you should throw
* away your reference to this instance whenever you receive that event.
* @extends EventEmitter
*/
class InspectorSession extends EventEmitter {
/**
* Binds the current inspector session to an Angular scope, meaning this inspector
* session will be closed as soon as the Angular scope gets destroyed.
* @param {object} scope - And angular scope object to bind to.
*/
public bindToAngularScope(scope: ng.IScope): void {
const removeWatch = scope.$on('$destroy', () => this.close());
this.on('closed', () => removeWatch());
}

/**
* Closes the opened inspector as long as it's stil the open one.
* If this is not the active session anymore, this method won't do anything.
* If this session was still active and an inspector was closed, the 'closed'
* event will be emitted on this InspectorSession instance.
*/
public close(): void {
if (activeSession === this) {
const container = document.getElementById(CONTAINER_ID);
if (container) {
ReactDOM.unmountComponentAtNode(container);
this.emit('closed');
}
}
}
}

/**
* Checks if a inspector panel could be shown based on the passed adapters.
*
Expand All @@ -98,6 +44,8 @@ interface InspectorOptions {
title?: string;
}

export type InspectorSession = FlyoutSession;

/**
* Opens the inspector panel for the given adapters and close any previously opened
* inspector panel. The previously panel will be closed also if no new panel will be
Expand All @@ -110,11 +58,6 @@ interface InspectorOptions {
* @return {InspectorSession} The session instance for the opened inspector.
*/
function open(adapters: Adapters, options: InspectorOptions = {}): InspectorSession {
// If there is an active inspector session close it before opening a new one.
if (activeSession) {
activeSession.close();
}

const views = viewRegistry.getVisible(adapters);

// Don't open inspector if there are no views available for the passed adapters
Expand All @@ -124,20 +67,9 @@ function open(adapters: Adapters, options: InspectorOptions = {}): InspectorSess
if an inspector can be shown.`);
}

const container = getOrCreateContainerElement();
const session = (activeSession = new InspectorSession());

ReactDOM.render(
<InspectorPanel
views={views}
adapters={adapters}
onClose={() => session.close()}
title={options.title}
/>,
container
);

return session;
return openFlyout(<InspectorPanel views={views} adapters={adapters} title={options.title} />, {
'data-test-subj': 'inspectorPanel',
});
}

const Inspector = {
Expand Down
Loading