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.

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

Add keyboard navigation #1908

Merged
merged 8 commits into from
Feb 4, 2022
Merged

Conversation

laliq
Copy link
Contributor

@laliq laliq commented Jan 24, 2022

Resolves #1901

@laliq
Copy link
Contributor Author

laliq commented Jan 24, 2022

I detect one 🐛

  1. When using arrows to focus the menu.item, the menu detaches itself from default position.

@oddvernes
Copy link
Collaborator

Actually, there is something quite funky going on, and I think pressing enter on the menu item does not count as a click after all. Going to the menu stories, onClick inside menuItem never registers anything when pressing enter (as opposed to clicking with mouse pinter), but console.log(new Error()) in the closeMenu function claims it was triggered by onClick on the anchor element button. I am a little bit confused. Suggest @mimarz also takes a look

@laliq
Copy link
Contributor Author

laliq commented Jan 24, 2022

Actually, there is something quite funky going on...

Yes i agree @oddvernes, and i'm a bit confuse too... Any way i will continue to fix this bug.

@oddvernes
Copy link
Collaborator

Maybe it should be its own ticket if the problem is deeper in the menu itself. And put this pr aside until it if fixed

@mimarz
Copy link
Contributor

mimarz commented Jan 25, 2022

Actually, there is something quite funky going on, and I think pressing enter on the menu item does not count as a click after all. Going to the menu stories, onClick inside menuItem never registers anything when pressing enter (as opposed to clicking with mouse pinter), but console.log(new Error()) in the closeMenu function claims it was triggered by onClick on the anchor element button. I am a little bit confused. Suggest @mimarz also takes a look

I am observing the same.

  • MenuItem.onClick is only triggered by pressing "space" key.
  • Anchor element (button) onClick is triggered when MenuItem is focused and key press.
    • Adding preventDefault to MenuList handleKeyPress stops this bubbling
  • This behaviour is probably a side-effect of trying to focus anchor element after Menu closes. Removing the globalKeyPress for Enter "fixes" this behaviour.

As @oddvernes suggested, we need an issue to fix this bug in Menu :)

Edit: Bug-fix issue #1910

@laliq laliq changed the title [WIP] Add keyboard navigation Add keyboard navigation Jan 27, 2022
@laliq laliq requested a review from oddvernes January 27, 2022 11:46
@laliq laliq requested review from oddvernes and mimarz and removed request for mimarz February 2, 2022 14:59
Copy link
Collaborator

@oddvernes oddvernes left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@laliq laliq merged commit 9cd0961 into develop Feb 4, 2022
@laliq laliq deleted the fix/ML-1901-kb-navigation-playground branch February 4, 2022 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No kb-navigation in Playground "settings" menu
3 participants