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

Refactor keycode exporting in ui_framework #13663

Merged
merged 5 commits into from
Aug 24, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 2 additions & 2 deletions src/core_plugins/kibana/public/dashboard/dashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { documentationLinks } from 'ui/documentation_links/documentation_links';
import { showCloneModal } from './top_nav/show_clone_modal';
import { migrateLegacyQuery } from 'ui/utils/migrateLegacyQuery';
import { QueryManagerProvider } from 'ui/query_manager';
import { ESC_KEY_CODE } from 'ui_framework/services';
import { keycodes } from 'ui_framework/services';
import { DashboardContainerAPI } from './dashboard_container_api';

const app = uiModules.get('app/dashboard', [
Expand Down Expand Up @@ -328,7 +328,7 @@ app.directive('dashboardApp', function ($injector) {
$scope.exitFullScreenMode = () => setFullScreenMode(false);

document.addEventListener('keydown', (e) => {
if (e.keyCode === ESC_KEY_CODE) {
if (e.keyCode === keycodes.ESCAPE) {
setFullScreenMode(false);
}
}, false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ uiModules.get('apps/management')
};

$scope.maybeCancel = function ($event, conf) {
if ($event.keyCode === keyCodes.ESC) {
if ($event.keyCode === keyCodes.ESCAPE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to replace the custom keyCodes instance here with import { keyCodes } from 'ui_framework/services';

$scope.cancelEdit(conf);
}
};
Expand Down
9 changes: 3 additions & 6 deletions src/ui/public/accessibility/__tests__/kbn_accessible_click.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ import sinon from 'sinon';
import expect from 'expect.js';
import ngMock from 'ng_mock';
import '../kbn_accessible_click';
import {
ENTER_KEY,
SPACE_KEY,
} from 'ui_framework/services';
import { keycodes } from 'ui_framework/services';

describe('kbnAccessibleClick directive', () => {
let $compile;
Expand Down Expand Up @@ -92,14 +89,14 @@ describe('kbnAccessibleClick directive', () => {

it(`on ENTER keyup`, () => {
const e = angular.element.Event('keyup'); // eslint-disable-line new-cap
e.keyCode = ENTER_KEY;
e.keyCode = keycodes.ENTER;
element.trigger(e);
sinon.assert.calledOnce(scope.handleClick);
});

it(`on SPACE keyup`, () => {
const e = angular.element.Event('keyup'); // eslint-disable-line new-cap
e.keyCode = SPACE_KEY;
e.keyCode = keycodes.SPACE;
element.trigger(e);
sinon.assert.calledOnce(scope.handleClick);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@ import sinon from 'sinon';
import expect from 'expect.js';
import ngMock from 'ng_mock';
import '../kbn_ui_ace_keyboard_mode';
import {
ENTER_KEY,
ESC_KEY_CODE,
} from 'ui_framework/services';
import { keycodes } from 'ui_framework/services';

describe('kbnUiAceKeyboardMode directive', () => {
let element;
Expand All @@ -30,7 +27,7 @@ describe('kbnUiAceKeyboardMode directive', () => {
const textarea = element.find('textarea');
sinon.spy(textarea[0], 'focus');
const ev = angular.element.Event('keydown'); // eslint-disable-line new-cap
ev.keyCode = ENTER_KEY;
ev.keyCode = keycodes.ENTER;
element.find('.uiAceKeyboardHint').trigger(ev);
expect(textarea[0].focus.called).to.be(true);
expect(element.find('.uiAceKeyboardHint').hasClass('uiAceKeyboardHint-isInactive')).to.be(true);
Expand All @@ -41,7 +38,7 @@ describe('kbnUiAceKeyboardMode directive', () => {
const hint = element.find('.uiAceKeyboardHint');
sinon.spy(hint[0], 'focus');
const ev = angular.element.Event('keydown'); // eslint-disable-line new-cap
ev.keyCode = ESC_KEY_CODE;
ev.keyCode = keycodes.ESCAPE;
textarea.trigger(ev);
expect(hint[0].focus.called).to.be(true);
expect(hint.hasClass('uiAceKeyboardHint-isInactive')).to.be(false);
Expand Down Expand Up @@ -80,7 +77,7 @@ describe('kbnUiAceKeyboardModeService', () => {
const textarea = element.find('textarea');
sinon.spy(textarea[0], 'focus');
const ev = angular.element.Event('keydown'); // eslint-disable-line new-cap
ev.keyCode = ENTER_KEY;
ev.keyCode = keycodes.ENTER;
element.find('.uiAceKeyboardHint').trigger(ev);
expect(textarea[0].focus.called).to.be(true);
expect(element.find('.uiAceKeyboardHint').hasClass('uiAceKeyboardHint-isInactive')).to.be(true);
Expand All @@ -91,7 +88,7 @@ describe('kbnUiAceKeyboardModeService', () => {
const hint = element.find('.uiAceKeyboardHint');
sinon.spy(hint[0], 'focus');
const ev = angular.element.Event('keydown'); // eslint-disable-line new-cap
ev.keyCode = ESC_KEY_CODE;
ev.keyCode = keycodes.ESCAPE;
textarea.trigger(ev);
expect(hint[0].focus.called).to.be(true);
expect(hint.hasClass('uiAceKeyboardHint-isInactive')).to.be(false);
Expand Down
4 changes: 2 additions & 2 deletions src/ui/public/accessibility/kbn_accessible_click.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

import {
accessibleClickKeys,
SPACE_KEY,
keycodes,
} from 'ui_framework/services';
import { uiModules } from 'ui/modules';

Expand All @@ -31,7 +31,7 @@ uiModules.get('kibana')
controller: $element => {
$element.on('keydown', e => {
// Prevent a scroll from occurring if the user has hit space.
if (e.keyCode === SPACE_KEY) {
if (e.keyCode === keycodes.SPACE) {
e.preventDefault();
}
});
Expand Down
6 changes: 3 additions & 3 deletions src/ui/public/accessibility/kbn_ui_ace_keyboard_mode.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import angular from 'angular';
import { uiModules } from 'ui/modules';
import './kbn_ui_ace_keyboard_mode.less';
import { ESC_KEY_CODE, ENTER_KEY } from 'ui_framework/services';
import { keycodes } from 'ui_framework/services';

let aceKeyboardModeId = 0;

Expand Down Expand Up @@ -53,7 +53,7 @@ uiModules.get('kibana')
}

hint.keydown((ev) => {
if (ev.keyCode === ENTER_KEY) {
if (ev.keyCode === keycodes.ENTER) {
ev.preventDefault();
startEditing();
}
Expand All @@ -64,7 +64,7 @@ uiModules.get('kibana')
});

uiAceTextbox.keydown((ev) => {
if (ev.keyCode === ESC_KEY_CODE) {
if (ev.keyCode === keycodes.ESCAPE) {
ev.preventDefault();
ev.stopPropagation();
enableOverlay();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,19 @@ import {
cloneElement,
} from 'react';

import {
ENTER_KEY,
SPACE_KEY,
} from '../../services';
import { keycodes } from '../../services';

export class KuiKeyboardAccessible extends Component {
onKeyDown = e => {
// Prevent a scroll from occurring if the user has hit space.
if (e.keyCode === SPACE_KEY) {
if (e.keyCode === keycodes.SPACE) {
e.preventDefault();
}
}

onKeyUp = e => {
// Support keyboard accessibility by emulating mouse click on ENTER or SPACE keypress.
if (e.keyCode === ENTER_KEY || e.keyCode === SPACE_KEY) {
if (e.keyCode === keycodes.ENTER || e.keyCode === keycodes.SPACE) {
// Delegate to the click handler on the element.
this.props.children.props.onClick(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@ import sinon from 'sinon';

import { KuiKeyboardAccessible } from './keyboard_accessible';

import {
ENTER_KEY,
SPACE_KEY,
} from '../../services';
import { keycodes } from '../../services';

describe('KuiKeyboardAccessible', () => {
describe('throws an error', () => {
Expand Down Expand Up @@ -176,7 +173,7 @@ describe('KuiKeyboardAccessible', () => {
);

$button.find('[data-div]').simulate('keyup', {
keyCode: ENTER_KEY
keyCode: keycodes.ENTER
});

sinon.assert.calledOnce(onClickHandler);
Expand All @@ -192,7 +189,7 @@ describe('KuiKeyboardAccessible', () => {
);

$button.find('[data-div]').simulate('keyup', {
keyCode: SPACE_KEY
keyCode: keycodes.SPACE
});

sinon.assert.calledOnce(onClickHandler);
Expand Down
4 changes: 2 additions & 2 deletions ui_framework/src/components/modal/confirm_modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { KuiModalHeaderTitle } from './modal_header_title';
import { KuiModalBody } from './modal_body';
import { KuiModalBodyText } from './modal_body_text';
import { KuiButton } from '../index';
import { ESC_KEY_CODE } from '../../services';
import { keycodes } from '../../services';

export const CONFIRM_BUTTON = 'confirm';
export const CANCEL_BUTTON = 'cancel';
Expand All @@ -32,7 +32,7 @@ export function KuiConfirmModal({

const onKeyDown = (event) => {
// Treat the 'esc' key as a cancel indicator.
if (event.keyCode === ESC_KEY_CODE) {
if (event.keyCode === keycodes.ESCAPE) {
onCancel();
}
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
export const ENTER_KEY = 13;
export const SPACE_KEY = 32;
import { ENTER, SPACE } from '../key_codes';

// These keys are used to execute click actions on interactive elements like buttons and links.
export const accessibleClickKeys = {
[ENTER_KEY]: 'enter',
[SPACE_KEY]: 'space',
[ENTER]: 'enter',
[SPACE]: 'space',
};
20 changes: 10 additions & 10 deletions ui_framework/src/services/accessibility/combo_box_key_codes.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@
*/

import {
DOWN_KEY_CODE,
ENTER_KEY_CODE,
ESC_KEY_CODE,
TAB_KEY_CODE,
UP_KEY_CODE,
DOWN,
ENTER,
ESCAPE as ESC,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to change this to just be ESCAPE and update the consumers of this service, for consistency?

TAB,
UP,
} from '../key_codes';

export const comboBoxKeyCodes = {
DOWN: DOWN_KEY_CODE,
ENTER: ENTER_KEY_CODE,
ESC: ESC_KEY_CODE,
TAB: TAB_KEY_CODE,
UP: UP_KEY_CODE,
DOWN,
ENTER,
ESC,
TAB,
UP,
};
7 changes: 1 addition & 6 deletions ui_framework/src/services/accessibility/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,2 @@
export {
accessibleClickKeys,
ENTER_KEY,
SPACE_KEY,
} from './accessible_click_keys';

export { accessibleClickKeys } from './accessible_click_keys';
export { comboBoxKeyCodes } from './combo_box_key_codes';
7 changes: 4 additions & 3 deletions ui_framework/src/services/index.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// Export all keycodes under a `keycodes` named variable
import * as keycodes from './key_codes';
export { keycodes };
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to keyCodes for consistent capitalization?


export {
accessibleClickKeys,
comboBoxKeyCodes,
ENTER_KEY,
SPACE_KEY,
} from './accessibility';
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the difference between ENTER_KEY from accessibility and keycodes.ENTER? Above you are importing import { keycodes, ENTER_KEY } from 'ui_framework/services'; which threw me off a bit. Seems like instead of ENTER_KEY it should just be keywords.ENTER

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Honestly I just refactored the key_codes file and left the accessible_click_keys as it was. I fixed that with now with another commit. Thanks.


export { SortableProperties } from './sort';
export { ESC_KEY_CODE } from './key_codes';

export { LEFT_ALIGNMENT, RIGHT_ALIGNMENT } from './alignment';
13 changes: 8 additions & 5 deletions ui_framework/src/services/key_codes.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
export const DOWN_KEY_CODE = 40;
export const ENTER_KEY_CODE = 13;
export const ESC_KEY_CODE = 27;
export const TAB_KEY_CODE = 9;
export const UP_KEY_CODE = 38;
export const ENTER = 13;
export const SPACE = 32;
export const ESCAPE = 27;
export const TAB = 9;

// Arrow keys
export const DOWN = 40;
export const UP = 38;