Added enum for triggering menu with arrow keys and bool to allow it or not#3950
Added enum for triggering menu with arrow keys and bool to allow it or not#3950dzearing merged 7 commits intomicrosoft:masterfrom eikawata:add-menu-trigger-button-configuration
Conversation
| @autobind | ||
| private _onMenuKeyDown(ev: React.KeyboardEvent<HTMLElement>) { | ||
| if (ev.which === KeyCodes.down) { | ||
| if (this.props.allowDirectionalMenuTrigger && ev.which === this._getDirectionKeyCode()) { |
There was a problem hiding this comment.
allowDirectionalMenuTrigger [](start = 19, length = 27)
I looked at the PR and was thinking if we actually need this boolean. Just the presence of the menuTriggerDirection is enough to confirm that the consumer wants to controll the key to use. No?
#Resolved
There was a problem hiding this comment.
What if we renamed this prop to menuTriggerKeyCode and simply use the KeyCode enum as its type?
#Resolved
There was a problem hiding this comment.
@manishgarg1 , My reasoning is that, since the current default behavior is to allow triggering menu with arrow down, I wanted to also enable it and set the default key to be arrow down. Hence, the bool value was needed to explicitly turn it off. I can of course make the prop KeyCodes | null, then disable it when it's set to null. But that sounds unintuitive when not specifying the prop defaults to arrow down.
@dzearing , I thought about that, but I personally felt being able to assign a random key to trigger a menu sounded odd, especially when pressing Enter/Space on a button already triggers a menu. If we are going to keep the bool value, then setting it to false still doesn't disable the Enter/Space key to open the menu.
There was a problem hiding this comment.
@ekawatani, exactly as you said. I would prefer "KeyCodes | undefined" as the API signature. And remove the bool. I will confirm with David if he is good with that. #Resolved
There was a problem hiding this comment.
@manishgarg1 , Thanks. I can remove the bool, and convert the other prop to use KeyCode as David suggested earlier. Just making sure, but we still want to maintain the default behavior to be keydown, correct? So, to disable the custom menu trigger would be to manually assign null to the prop.
Let me know if this is what we want, and I'll send an update. #Resolved
There was a problem hiding this comment.
Hi @ekawatani , Yes we would like the default behaviour to be keydown. The default value for your new property would be 'undefined' if the consumer code does not set that property. Does that explain the expected behaviour?
Further, just rename the prop to menuTriggerKeyCode as David mentioned above. #Resolved
…button-configuration
…button-configuration
|
|
||
| /** | ||
| * Defines a custom key that opens button menu. If the value is not provided, down arrow key is used. | ||
| * If set to null, this will be disabled. |
There was a problem hiding this comment.
Provides a custom KeyCode that can be used to open the button menu. The default keyCode is the down arrow. A value of null can be provided to disable the key codes for opening the button menu.
|
Resetting the build... |
* master: (71 commits) Applying package updates. Delete initials_2018-02-07-13-49.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Update .npmrc Cleaning Up Console Log in CommandBar Test (microsoft#4011) Convert Overlay to mergeStyles (microsoft#3978) ScrollablePane SCSS to MergeStyles Part 1: File Structure (microsoft#4008) [ContextualMenu] Fixes useTargetWidth property (microsoft#3943) DetailsList: Consider groups when setting aria-rowcount List: Add a _notifyPageChanges function (microsoft#3990) Applying package updates. Migrating Coachmark to main Package, Added a beak component and updated experiment PositioningContainer. (microsoft#3919) Update package.json Added enum for triggering menu with arrow keys and bool to allow it or not (microsoft#3950) BaseExtendedPicker: Hook up onPaste (microsoft#3885) FocusUtil: fix getPreviousElement to include previous sibling elements correctly (microsoft#3928) ...
3 participants
Pull request checklist
$ npm run changeDescription of changes
Added a enum for specifying which arrow key is used to trigger button menu. Also, added a boolean value indicating whether triggering menu using arrow keys is allowed.
Focus areas to test
Ensure button menu can open using keyboards. Also, verify split buttons behaviors didn't regress.