-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
@@ -6,6 +10,5 @@ export { | |||
} from './accessibility'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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.
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.
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) { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
@cjcenizal implemented all your feedback. Could you give it another review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚽️ LGTM!
* 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_KEY
vsESC_KEY_CODE
inservices/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.js
file will now be exported fromservices/index.js
with 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).