Skip to content

Conversation

@evgenoid
Copy link
Contributor

@evgenoid evgenoid commented Oct 2, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Issue #309

What is the current behavior?

No way to navigate through Menu/List with the keyboard

What is the new behavior?

Added ability to navigate through Menu/List with the keyboard

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Screen.Recording.2025-10-02.at.13.38.09.mov

@evgenoid evgenoid self-assigned this Oct 2, 2025
@netlify
Copy link

netlify bot commented Oct 2, 2025

Deploy Preview for reablocks-storybook ready!

Name Link
🔨 Latest commit 352b477
🔍 Latest deploy log https://app.netlify.com/projects/reablocks-storybook/deploys/68df8330df18ff00084db35c
😎 Deploy Preview https://deploy-preview-310--reablocks-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.


const onKeyDown = useCallback((e: KeyboardEvent) => {
e.stopPropagation();
if (e.key === 'ArrowLeft' || e.key === 'Escape') {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't escape close the menu all together? I know the ExitListener often is handling this. Just calling this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is closing, but all levels together. But we need to give user ability get back to previous level

const theme: ListTheme = useComponentTheme('list', customTheme);
const containerRef = useRef<HTMLDivElement>(null);

const handleKeyDown = (event: React.KeyboardEvent<HTMLDivElement>) => {
Copy link
Member

Choose a reason for hiding this comment

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

Im not sure I agree this should be in List ( or at least not in the base list component ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not in the List? It is have the same functionality as menu and user should have ability to navigate it in the same way/

Copy link
Contributor Author

@evgenoid evgenoid Oct 2, 2025

Choose a reason for hiding this comment

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

Also, it works only for items with tab index (simplly say which have onClick callback)

@evgenoid evgenoid mentioned this pull request Oct 3, 2025
2 tasks
Copy link
Member

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

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

I think we should make a MenuList and MenuListItem that wraps the base List/ListItem and attaches these. Thoughts?

@evgenoid
Copy link
Contributor Author

evgenoid commented Oct 6, 2025

I think we should make a MenuList and MenuListItem that wraps the base List/ListItem and attaches these. Thoughts?

If we don't need navigation in the List component then make sense.

@amcdnl
Copy link
Member

amcdnl commented Oct 6, 2025

I think we should make a MenuList and MenuListItem that wraps the base List/ListItem and attaches these. Thoughts?

If we don't need navigation in the List component then make sense.

Lists don't need keyboard nav like this since they have tab navigation natively.

@evgenoid
Copy link
Contributor Author

evgenoid commented Oct 6, 2025

I think we should make a MenuList and MenuListItem that wraps the base List/ListItem and attaches these. Thoughts?

If we don't need navigation in the List component then make sense.

Lists don't need keyboard nav like this since they have tab navigation natively.

Ok, will update.

@amcdnl
Copy link
Member

amcdnl commented Oct 6, 2025

I think we should make a MenuList and MenuListItem that wraps the base List/ListItem and attaches these. Thoughts?

If we don't need navigation in the List component then make sense.

Lists don't need keyboard nav like this since they have tab navigation natively.

Ok, will update.

Thanks. Once done, don't forget to add docs about this.

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.

3 participants