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 selection #1918

Merged
merged 5 commits into from
Feb 1, 2022
Merged

Conversation

laliq
Copy link
Contributor

@laliq laliq commented Jan 26, 2022

Resolves #1910

@laliq laliq marked this pull request as ready for review January 27, 2022 07:56
@laliq laliq changed the title 🚧 Add keyboard selection Add keyboard selection Jan 27, 2022
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.

Currently the keypress only handles onclose, which only closes the menu without selecting anything. But it is onClick we need. This is what is used to call different functions. The problem is onClick expects a mouseevent . We need enter and click to behave exactly the same.
My suggestion was to just dispatch a click event when you press enter, which does seem to work perfectly
if (key === 'Enter') { e.target.dispatchEvent(new Event('click', { bubbles: true })) }
But maybe @mimarz have some some reservations?:P

Also in menu stories something is trapping focus on the anchor button while tabbing, but this seems to be an existing bug, having to do with event.preventDefaut/stopPropagation in the stories maybe

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.

We need to make sure the expected behaviour is the same for when either click or keypress is used. That being that the onClose handler passed to Menu is called with the event.

Introducing an onClose on MenuItem in addition to the on one Menu would cause confusion as on or the other would be called be it click or keypress.

packages/eds-core-react/src/components/Menu/MenuItem.tsx Outdated Show resolved Hide resolved
packages/eds-core-react/src/components/Menu/MenuItem.tsx Outdated Show resolved Hide resolved
@mimarz
Copy link
Contributor

mimarz commented Jan 27, 2022

Currently the keypress only handles onclose, which only closes the menu without selecting anything. But it is onClick we need. This is what is used to call different functions. The problem is onClick expects a mouseevent . We need enter and click to behave exactly the same. My suggestion was to just dispatch a click event when you press enter, which does seem to work perfectly if (key === 'Enter') { e.target.dispatchEvent(new Event('click', { bubbles: true })) } But maybe @mimarz have some some reservations?:P

Also in menu stories something is trapping focus on the anchor button while tabbing, but this seems to be an existing bug, having to do with event.preventDefaut/stopPropagation in the stories maybe

Not sure, mimicing a click event when a key is pressed feels very wrong :P Typing should not be a problem as we can just use "or" on the event type?

@oddvernes
Copy link
Collaborator

Not sure, mimicing a click event when a key is pressed feels very wrong :P Typing should not be a problem as we can just use "or" on the event type?

onClick?: (e: React.MouseEvent | React.KeyboardEvent) => void

if (key === 'Enter') { if (onClick) { onClick(e) }

Yes this works if you also declare the event as mouse or keyboard downstream in the stories (I don't know how many people even use this event in their apps, so its only a small breaking change for those that do)

@mimarz
Copy link
Contributor

mimarz commented Jan 27, 2022

I was thinking more for the onClose handler? Lets not mess expected native behaviours on handler 😅

@laliq
Copy link
Contributor Author

laliq commented Jan 27, 2022

Thanks guys 🙏

@laliq
Copy link
Contributor Author

laliq commented Jan 28, 2022

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 👍
I'll make a new issue for focus getting stuck on the anchor button in menu stories

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.

Quirky solution! LGTM 👍

@laliq laliq merged commit 1b8f87a into develop Feb 1, 2022
@laliq laliq deleted the feature/ML-1910-menuitem-keyboard-select branch February 1, 2022 07:38
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.

MenuItem keyboard selection
3 participants