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

feat(overlay-dropdown): add arrow key navigation #2679

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jan 17, 2022
return (
<li onClick={ onClick }>
<li role="menuitem" onClick={ onClick } onKeyDown={ handleKeydown }>
<button type="button" title={ text }>
Copy link
Member

Choose a reason for hiding this comment

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

Consider making the button the clickable element (for A11y purposes).

Copy link
Member

Choose a reason for hiding this comment

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

Consider making the key-up-down-to-change selection a DOM-only / no react directive:

This would simplify it and allow us to re-use it in other areas easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually come from this direction in a first iteration but went away from it.

Simple reason: getting away from React made it harder to test, firing the native events instead of simulating it in our existing Enzyme infrastructure. Adding it via onKeyDown made it much easier to test.

@pinussilvestrus pinussilvestrus marked this pull request as ready for review January 17, 2022 11:17
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 17, 2022
@pinussilvestrus pinussilvestrus requested review from a team, philippfromme, smbea and andreasgeier and removed request for a team January 17, 2022 11:17
@pinussilvestrus
Copy link
Contributor Author

@andreasgeier I know there are a couple of reviews assigned to you currently, so I don't want to block you with this if it's not necessary. It's up to you whether you want to have a look UI wise.

Copy link

@andreasgeier andreasgeier left a comment

Choose a reason for hiding this comment

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

✅ Works as expected

@MaxTru MaxTru added this to the M52 milestone Jan 18, 2022
@MaxTru
Copy link
Contributor

MaxTru commented Jan 18, 2022

Marking this for M52 milestone, since we mention it in the blog post

@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Jan 21, 2022

I rebased this on top of the latest develop changes. This is (still) ready for review though.

Copy link
Contributor

@MaxTru MaxTru left a comment

Choose a reason for hiding this comment

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

Really nice.

I guess this is still for discussion? #2679 (comment)

But apart from that - LGTM

@pinussilvestrus
Copy link
Contributor Author

I guess this is still for discussion? #2679 (comment)

Yes, but I probably won't get back to it during spring cleaning. I am happy we have this as a built-in feature for all overlay dropdowns now, and also easily tested. We can reconsider making it an even more generic feature in the future, if we want to.

@pinussilvestrus pinussilvestrus merged commit ae72af9 into develop Jan 21, 2022
@pinussilvestrus pinussilvestrus deleted the 2626-navigate-with-arrow-keys branch January 21, 2022 08:37
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 21, 2022
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.

Navigate overlay dropdowns via arrow keys
4 participants