Improve KuiContextMenu keyboard navigation UX #44
Conversation
There was a problem hiding this comment.
The JS in this is beyond my abilities for review, but the functionality seems to still work. I noticed a small tabbing issue on the last panel and left a few minor comments. Looks good.
Minor tabbing issue that I have trouble replicating...
Note that the title wasn't tabbable until the last attempt (I shift-tabbed to get to it). Also, it's hard to see in the gif, but when it'd tab to the button, it would automatically tab back to the switch (skipping the the title link). Tried another time to replicate this, but couldn't get it running again.
| @@ -12,6 +12,10 @@ $euiContextMenuWidth: $euiSize * 16; | |||
| } | |||
| } | |||
|
|
|||
| .euiContextMenu__panel { | |||
There was a problem hiding this comment.
This looks similarly named to .euiContextMenuPanel. Assume they're both needed?
There was a problem hiding this comment.
Yep, because the ContextMenu is what you'll use when there are multiple panels, and it will need to absolutely position them. But when you're just using a ContextMenuPanel by itself, it doesn't need to be absolutely positioned (and shouldn't be, because it will break the layout). I'll leave a comment.
| anchorPosition="downLeft" | ||
| > | ||
| <EuiContextMenuPanel | ||
| title="Options" |
There was a problem hiding this comment.
Docs for this example say The below pagination example has no nesting and no title. Might want to remove the title here. Generally I think it's a little redundant functionally. I know I need to make it so the arrow colors correctly when no title is present.
There was a problem hiding this comment.
Made an issue for myself for the coloring. #45
docs/src/views/popover/popover.js
Outdated
| @@ -41,6 +41,7 @@ export default class extends Component { | |||
|
|
|||
| return ( | |||
| <EuiPopover | |||
| isFocusable | |||
There was a problem hiding this comment.
Neat! Can you document this prop somehow?
There was a problem hiding this comment.
Good catch! Quick q for you... based on the name, what do you think this prop does? I'm pretty sure I picked an atrocious name.
There was a problem hiding this comment.
Just knowing how that thing works I'd assume it says whether the content inside traps focus or something?
There was a problem hiding this comment.
Haha yeah I picked a bad name. You need to specify it as true only if there is no content inside which is focusable! So maybe I should rename this to nonFocusable or isNotFocusable. Or think of a solution which makes this prop unnecessary.
|
Thanks for the great review @snide! I fixed that tabbing bug and your other feedback. I'll address the isFocusable examples in a subsequent commit once we figure out if it's a suitable name. |
|
Added some commits to port over elastic/kibana#14572 and elastic/kibana#14617 |
|
@snide Could you take another look and give a +1? |
* Refactor focus state logic to use the React lifecycle correctly. * Update KuiPopover snapshots. * Remove unnecessary isVisible prop from KuiContextMenu. * Allow user to both tab AND use the arrow keys for navigation. * Reinstate ability to tab and shift-tab to the title of KuiContextMenuPanel. * Release focus from Dashboard panel options KuiContextMenu by closing it when you select an option. * Update KuiContextMenu example to demonstrate best practice of closing the menu when an item is clicked. * Replace native transitionend event handler with onAnimationEnd React event handler.
- Support left arrow going back when the panel itself is focused. [UI Framework] Fix Popover and ContextMenu bugs. * Rename KuiPopover isFocusable prop to ownFocus. Focus on first focusable element by default. * Fix bug where ContextMenuPanel keyboard navigation broke if the user was using tab instead of arrow keys.
`colorMode` key improvements; `computed` dependencies update
Ports over improvements from elastic/kibana#14434: