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

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Aug 23, 2017

I refactored the exporting of the keycodes in the UI framework.

Why? We currently export keycodes with several different names ENTER_KEY vs ESC_KEY_CODE in services/index.js. Also when adding new keycodes you would always need to reexport them manually in the services/index.js.

With this PR all keycodes described in the key_codes.js file will now be exported from services/index.js with the name keycodes. That way also the service import won't be "polluted" with "lots of" keycodes, if you happen to import, the whole service (which you actually shouldn't).

@timroes timroes added the Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. label Aug 23, 2017
@timroes timroes added the review label Aug 23, 2017
@@ -6,6 +10,5 @@ export {
} 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.

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

Sweet. Didn't test locally, but I like the changes.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

This is a nice improvement! I found one bug and had a couple small suggestions.

@@ -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?

@@ -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';

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?

@timroes
Copy link
Contributor Author

timroes commented Aug 24, 2017

@cjcenizal implemented all your feedback. Could you give it another review?

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

⚽️ LGTM!

@timroes timroes merged commit 3b1e1e5 into elastic:master Aug 24, 2017
timroes added a commit that referenced this pull request Aug 24, 2017
* Refactor keycode exporting in ui_framework

* Replace all uses of ENTER_KEY and SPACE_KEY

* Use ESCAPE instead of ESC universally

* Rename keycodes -> keyCodes

* Replace keyCodes in advanced_row
@timroes timroes deleted the refactor-keycodes branch August 24, 2017 19:26
@timroes
Copy link
Contributor Author

timroes commented Aug 24, 2017

Backports:

@timroes timroes removed the review label Nov 17, 2017
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Nov 20, 2017
* Refactor keycode exporting in ui_framework

* Replace all uses of ENTER_KEY and SPACE_KEY

* Use ESCAPE instead of ESC universally

* Rename keycodes -> keyCodes

* Replace keyCodes in advanced_row
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Dec 1, 2017
* Refactor keycode exporting in ui_framework

* Replace all uses of ENTER_KEY and SPACE_KEY

* Use ESCAPE instead of ESC universally

* Rename keycodes -> keyCodes

* Replace keyCodes in advanced_row
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v6.1.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants