Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[EuiContextMenuPanelDescriptor] Add initialFocusedItemIndex support #4223

Merged
merged 7 commits into from
Nov 4, 2020
Merged

[EuiContextMenuPanelDescriptor] Add initialFocusedItemIndex support #4223

merged 7 commits into from
Nov 4, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Nov 3, 2020 •
edited
Loading

Summary

Closes #4216 by adding support for initialFocusedItemIndex when using the EuiContextMenuPanelDescriptor approach.
Rather than reserve setting focus to situations of keyboard navigation, EuiContextMenu will now respect setting focus when specified as well as use the value for initial focus in situations of keyboard navigation.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox

- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4223/

chandlerprall
chandlerprall suggested changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change works for me. One very small change requested.

src/components/context_menu/context_menu.tsx Outdated Show resolved Hide resolved
chandlerprall
chandlerprall approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉 (needs changelog)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4223/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4223/

@thompsongl
Copy link
Contributor Author

jenkins test this

Error: Timed out waiting for: http-get://localhost:9999

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4223/

@flash1293
Copy link
Contributor

Wow, that was quick - thanks @thompsongl for picking it up! I know I didn't mention this explicitly in the issue, but it seems like with this approach it's not possible to set initialFocusedItemIndex to undefined, right? Does this case make sense? In the linked cross-issue in Kibana (elastic/kibana#81058) my use case is to not select any item in the opened menu panel, but focus the panel itself.

@thompsongl
Copy link
Contributor Author

@flash1293 Ok, I think I fully understand your use case now.
What about using initialFocusedItemIndex={-1} to specify selecting the panel element? I'd just need to make one update to focus logic.

@flash1293
Copy link
Contributor

@thompsongl Sounds great to me

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4223/

chandlerprall
chandlerprall approved these changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4223/

@thompsongl thompsongl merged commit 33d61bb into elastic:master Nov 4, 2020
cchaos pushed a commit to andreadelrio/eui that referenced this pull request Nov 4, 2020
@thompsongl
[EuiContextMenuPanelDescriptor] Add initialFocusedItemIndex support (…
17d6823 
…elastic#4223)

* respect initialFocusedItemIndex in EuiContextMenuPanelDescriptor

* add initialFocusedItemIndex example

* nullish coalescing operator

* CL

* -1 focuses the panel
@chandlerprall chandlerprall mentioned this pull request Nov 23, 2020
Closed
@chandlerprall chandlerprall mentioned this pull request Dec 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

@flash1293 flash1293 Awaiting requested review from flash1293

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EuiContextMenuPanelDescriptor] Allow specifying initialFocusedItemIndex for panel
4 participants
@thompsongl @kibanamachine @flash1293 @chandlerprall