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

});