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

Uses new WindowEvent component for Flyout "close on ESC" #1127

Merged
merged 11 commits into from
Aug 22, 2018
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
## [`master`](https://github.com/elastic/eui/tree/master)

- Added a new `EuiWindowEvent` component for declarative, safe management of `window` event listeners
- Changed `Flyout` component to close on ESC keypress even if the flyout does not have focus

## [`master`](https://github.com/elastic/eui/tree/master)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copied heading, you can just append your bullets after the EuiSuperSelect one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed here: 73f0ed0


- Added `zIndexAdjustment` to `EuiPopover` which allows tweaking the popover content's `z-index` ([#1097](https://github.com/elastic/eui/pull/1097))
- Added new `EuiSuperSelect` component and `hasArrow` prop to `EuiPopover` ([#921](https://github.com/elastic/eui/pull/921))

Expand Down
4 changes: 4 additions & 0 deletions src-docs/src/routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,9 @@ import { ToolTipExample }
import { ToggleExample }
from './views/toggle/toggle_example';

import { WindowEventExample }
from './views/window_event/window_event_example';

import { XYChartExample }
from './views/series_chart/series_chart_example';

Expand Down Expand Up @@ -395,6 +398,7 @@ const navigation = [{
ToggleExample,
UtilityClassesExample,
MutationObserverExample,
WindowEventExample,
].map(example => createExample(example)),
}, {
name: 'Package',
Expand Down
51 changes: 51 additions & 0 deletions src-docs/src/views/window_event/window_event.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import React, { Component } from 'react';

import {
EuiWindowEvent,
EuiIcon,
EuiButton,
EuiSpacer,
} from '../../../../src/components';

export class WindowEvent extends Component {
constructor(props) {
super(props);

this.state = {
stars: []
};
this.add = this.add.bind(this);
this.remove = this.remove.bind(this);
}

add() {
this.setState((state) => ({ stars: [...state.stars, <EuiIcon type="starEmpty" />] }));
}

remove({ key }) {
if (key === 'Backspace' || key === 'Delete') {
this.setState((state) => ({ stars: state.stars.slice(0, -1) }));
return;
}
}

render() {
const { stars } = this.state;
return (
<div>
<EuiWindowEvent event="keydown" handler={this.remove} />
<EuiButton onClick={this.add}>Add a Star</EuiButton>

<EuiSpacer size="m" />

<p>To remove a star, press the backspace or delete key.</p>

<EuiSpacer size="m" />

<div className="stars-container">
{stars.map((star, i) => <span key={i} style={{ marginRight: '5px' }}>{star}</span>)}
</div>
</div>
);
}
}
37 changes: 37 additions & 0 deletions src-docs/src/views/window_event/window_event_example.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import React from 'react';

import { renderToHtml } from '../../services';

import {
GuideSectionTypes,
} from '../../components';

import {
EuiCode,
EuiWindowEvent,
} from '../../../../src/components';

import { WindowEvent } from './window_event';
const source = require('!!raw-loader!./window_event');
const html = renderToHtml(WindowEvent);

export const WindowEventExample = {
title: 'Window Event',
sections: [{
title: 'Window Event',
source: [{
type: GuideSectionTypes.JS,
code: source,
}, {
type: GuideSectionTypes.HTML,
code: html,
}],
text: (
<p>
Use an <EuiCode>EuiWindowEvent</EuiCode> to safely manage adding and auto-removing event listeners to <EuiCode>window</EuiCode>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also describe how?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed: e4d90d9

</p>
),
components: { EuiWindowEvent },
demo: <WindowEvent />,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a props key here to populate a props tab with the props listed in a table like so:
props: { EuiWindowEvent }

}],
};
4 changes: 2 additions & 2 deletions src/components/flyout/flyout.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { keyCodes } from '../../services';

import { EuiOverlayMask } from '../overlay_mask';
import { EuiButtonIcon } from '../button';
import { EuiWindowEvent } from '../window_event';

const sizeToClassNameMap = {
s: 'euiFlyout--small',
Expand All @@ -20,7 +21,6 @@ export class EuiFlyout extends Component {
onKeyDown = event => {
if (event.keyCode === keyCodes.ESCAPE) {
event.preventDefault();
event.stopPropagation();
this.props.onClose();
}
};
Expand Down Expand Up @@ -72,7 +72,6 @@ export class EuiFlyout extends Component {
}}
className={classes}
tabIndex={0}
onKeyDown={this.onKeyDown}
style={newStyle || style}
{...rest}
>
Expand All @@ -90,6 +89,7 @@ export class EuiFlyout extends Component {

return (
<span>
<EuiWindowEvent event="keydown" handler={this.onKeyDown} />
{optionalOverlay}
{/* Trap focus even when ownFocus={false}, otherwise closing the flyout won't return focus
to the originating button */}
Expand Down
4 changes: 4 additions & 0 deletions src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ export {
EuiToolTip,
} from './tool_tip';

export {
EuiWindowEvent
} from './window_event';

export {
EuiHideFor,
EuiShowFor,
Expand Down
1 change: 1 addition & 0 deletions src/components/window_event/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as EuiWindowEvent } from './window_event';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the default as here as it's in the component file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how you "proxy export" a default component from another file, I think. Or I could make the export in window_event be named and drop default as here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we just name everything so we ensure there are no conflicts or that we can add to them down the line without breaking changes.

51 changes: 51 additions & 0 deletions src/components/window_event/window_event.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { Component } from 'react';
import PropTypes from 'prop-types';

/**
* Adds and removes window events for you (renders null)
* Usage:
* <WindowEvent event='keydown' handler={this.handleKeyDown} />
*/
export default class WindowEvent extends Component {

componentDidMount() {
this.addEvent(this.props);
}

componentDidUpdate(prevProps) {
if (prevProps.event !== this.props.event || prevProps.handler !== this.props.handler) {
this.removeEvent(prevProps);
this.addEvent(this.props);
}
}

componentWillUnmount() {
this.removeEvent(this.props);
}

addEvent({ event, handler }) {
window.addEventListener(event, handler);
}

removeEvent({ event, handler }) {
window.removeEventListener(event, handler);
}

render() {
return null;
}

}

WindowEvent.displayName = 'WindowEvent';

WindowEvent.propTypes = {
/**
* Type of event
*/
event: PropTypes.string.isRequired,
/**
* Event callback function
*/
handler: PropTypes.func.isRequired
};
53 changes: 53 additions & 0 deletions src/components/window_event/window_event.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import React from 'react';
import { shallow } from 'enzyme';
import { EuiWindowEvent } from '.';

describe('EuiWindowEvent', () => {

beforeEach(() => {
window.addEventListener = jest.fn();
window.removeEventListener = jest.fn();
});

afterEach(() => {
jest.restoreAllMocks();
});

test('attaches handler to window event on mount', () => {
const handler = () => null;
shallow(<EuiWindowEvent event="click" handler={handler} />);
expect(window.addEventListener).toHaveBeenCalledTimes(1);
expect(window.addEventListener).toHaveBeenCalledWith('click', handler);
});

test('removes handler on unmount', () => {
const handler = () => null;
const wrapper = shallow(<EuiWindowEvent event="click" handler={handler} />);
wrapper.unmount();
expect(window.removeEventListener).toHaveBeenLastCalledWith('click', handler);
});

test('removes and re-attaches handler to window event on update', () => {
const handler1 = () => null;
const handler2 = () => null;
const wrapper = shallow(<EuiWindowEvent event="click" handler={handler1} />);

expect(window.addEventListener).toHaveBeenLastCalledWith('click', handler1);

wrapper.setProps({ event: 'hover', handler: handler2 });

expect(window.removeEventListener).toHaveBeenLastCalledWith('click', handler1);
expect(window.addEventListener).toHaveBeenLastCalledWith('hover', handler2);
});

test('does not remove or re-attach handler if update is irrelevant', () => {
const handler = () => null;
const wrapper = shallow(<EuiWindowEvent event="click" handler={handler} />);
expect(window.addEventListener).toHaveBeenCalledTimes(1);

wrapper.setProps({ whatever: 'ugh' });
expect(window.addEventListener).toHaveBeenCalledTimes(1);
expect(window.removeEventListener).not.toHaveBeenCalled();
});

});