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

Match focus order to visual order #827

Merged
merged 3 commits into from
Sep 21, 2024
Merged

Conversation

t-hamano
Copy link
Contributor

Proposed solution

Thank you for providing an amazing library.

I'm a member of the Gutenberg project (part of WordPress) and provide our own ResizableBox component that wraps this library.

Our own component supports the same props as the library, and additionally renders a focusable button element via the handleComponent prop to allow the user to focus the resizer and resize it via keyboard events (Example).

I noticed that because the Resizer components are always rendered together after its children, sometimes the focus order doesn't match the visual order.

This PR splits the renderResizer() into before and after the children, which should make the focus order and visual order match.

Testing Done

I've created a temporary story to test this PR, and if it makes sense I'd like to delete it after you review it.

The URL for the storybook is http://localhost:6066/?path=/story/handle--multiple.

The video below shows how pressing the tab key moves focus when children contains focusable elements:

Before

before.mp4

After

after.mp4

@bokuweb
Copy link
Owner

bokuweb commented Sep 21, 2024

@t-hamano Thanks for your great work.
However test is failed now, could you please check it 🙏
https://github.com/bokuweb/re-resizable/actions/runs/10962662887/job/30466650429?pr=827

@t-hamano
Copy link
Contributor Author

Thanks for the review! I should have run the unit tests locally before submitting the PR 😅

The unit test was failing because splitting the resizer increased the number of divs by one:

Before

before

After

after

Therefore, it seems the test fails because the number of actual div elements doesn't match the number of divs the toBe() matcher expects.

The increase in the number of divs was intentional, so I fixed the test itself.

For the same reason, the second div, (await divs.all())[1], is no longer a resizer. So it also needs to be increased by one.

Please let me know if I've missed anything.

@bokuweb
Copy link
Owner

bokuweb commented Sep 21, 2024

@t-hamano Looks great!! thanks for your contribution :)

@bokuweb bokuweb merged commit 3f15fca into bokuweb:master Sep 21, 2024
3 checks passed
@bokuweb
Copy link
Owner

bokuweb commented Sep 21, 2024

6.10.0 published

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.

2 participants