Refactor keycode exporting in ui_framework#13663
Conversation
| @@ -6,6 +10,5 @@ export { | |||
| } from './accessibility'; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
stacey-gammon
left a comment
There was a problem hiding this comment.
Sweet. Didn't test locally, but I like the changes.
cjcenizal
left a comment
There was a problem hiding this comment.
This is a nice improvement! I found one bug and had a couple small suggestions.
ui_framework/src/services/index.js
Outdated
| @@ -1,11 +1,12 @@ | |||
| // Export all keycodes under a `keycodes` named variable | |||
| import * as keycodes from './key_codes'; | |||
| export { keycodes }; | |||
There was a problem hiding this comment.
Can we rename this to keyCodes for consistent capitalization?
|
|
||
| $scope.maybeCancel = function ($event, conf) { | ||
| if ($event.keyCode === keyCodes.ESC) { | ||
| if ($event.keyCode === keyCodes.ESCAPE) { |
There was a problem hiding this comment.
We need to replace the custom keyCodes instance here with import { keyCodes } from 'ui_framework/services';
| UP_KEY_CODE, | ||
| DOWN, | ||
| ENTER, | ||
| ESCAPE as ESC, |
There was a problem hiding this comment.
Do you want to change this to just be ESCAPE and update the consumers of this service, for consistency?
|
@cjcenizal implemented all your feedback. Could you give it another review? |
* 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
|
Backports:
|
I refactored the exporting of the keycodes in the UI framework.
Why? We currently export keycodes with several different names
ENTER_KEYvsESC_KEY_CODEinservices/index.js. Also when adding new keycodes you would always need to reexport them manually in theservices/index.js.With this PR all keycodes described in the
key_codes.jsfile will now be exported fromservices/index.jswith the namekeycodes. 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).