refactor: Use localizable messages. #458
Conversation
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @gonfunko! LGTM, though I did have some thoughts on the messages naming (though that would presumably require changes here & in v12, so I defer to your judgment on how you want to proceed).
| const oldDisplayText = this.oldContextMenuItem | ||
| .displayText as DisplayTextFn; | ||
| return oldDisplayText(scope) + ' (Del)'; | ||
| return oldDisplayText(scope) + ` (${shortcut})`; |
There was a problem hiding this comment.
Will this localize for RTL correctly?
There was a problem hiding this comment.
No, but this is kind of a hack in the first place so I think that's acceptable for now.
src/actions/enter.ts
Outdated
| if (!this.tryShowFullBlockFieldEditor(block)) { | ||
| const shortcut = getShortActionShortcut('list_shortcuts'); | ||
| const message = `Press ${shortcut} for help on keyboard controls`; | ||
| const message = Msg['HELP_PROMPT'].replace('%1', shortcut); |
There was a problem hiding this comment.
Here & elsewhere: how descriptive do the names need to be? If they are global context, I'd expect something like: KEYBOARD_SHORTCUTS_HELP_PROMPT to be much clearer and avoid potential duplications in the future. If this level of specificity is preferred, can all of the other ones be updated accordingly?
There was a problem hiding this comment.
...I realize we just release core, so we're kind of committed now. Sorry I missed this until I went looking for things to pick up now that that was out the door :/
|
I think this needs one more rebase. |
| private shortcutNames: string[] = []; | ||
| private menuItemNames: string[] = []; | ||
|
|
||
| private registerShortcuts() { | ||
| const shortcuts: ShortcutRegistry.KeyboardShortcut[] = [ | ||
| // Begin and end move. | ||
| { |
There was a problem hiding this comment.
Moving the list of shortcuts from the class definition into a method makes it impossible to reference them from elsewhere, obliging you to add a separate private member just the shortcuts can later be unregistered. This is inelegant and complicates certain other refactors, such as making this horrid bit of code a bit less opaque by having it actually reference the keybindings used by these shortcuts.
Since you are not an idiot, I presume you must have had a good reason to do this.
One obvious good reason would be to delay evaluation of the Msg[…] lookups until after messages are loaded. But I cannot see any point in time between when the MoveActions constructor is called and when moveActions.init() is called when it is possible to load messages, since both things happen as a result of the construction of the NavigationController instance.
What am I missing here?
This PR fixes #412 by updating the keyboard experimentation repo to reference localizable strings from core Blockly instead of hard coding them.