-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
chore: Add focus-trap
example
#988
Conversation
This pull request is being automatically deployed with Vercel (learn more). reakit – ./🔍 Inspect: https://vercel.com/ariakit/reakit/D9YBjAusWxkAFB9v2K2ABwtKEjf6 [Deployment for f858069 canceled] ariakit – ./🔍 Inspect: https://vercel.com/ariakit/ariakit/4teJa7pUCjCc3EPVTJbGm2fLUxY9 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Codecov Report
@@ Coverage Diff @@
## main #988 +/- ##
==========================================
+ Coverage 65.06% 69.26% +4.19%
==========================================
Files 174 179 +5
Lines 4311 4402 +91
Branches 1186 1206 +20
==========================================
+ Hits 2805 3049 +244
+ Misses 1505 1352 -153
Partials 1 1
Continue to review full report at Codecov.
|
Great job! I'd suggest we make this example more simple though. And let users disable the focus trap behavior so keyboard users accessing the docs will be able to use the site. Let's also avoid using Wrote this snippet to illustrate what I'm talking about: import { FocusEvent, useRef } from "react";
import { useCheckboxState, Checkbox, CheckboxCheck } from "ariakit/checkbox";
import { FocusTrap } from "ariakit/focus-trap";
import "./style.css";
export default function Example() {
const focusTrapped = useCheckboxState({ defaultValue: true });
const firstRef = useRef<HTMLButtonElement>(null);
const lastRef = useRef<HTMLButtonElement>(null);
const onTrapFocus = (event: FocusEvent) => {
if (event.relatedTarget === firstRef.current) {
lastRef.current?.focus();
} else {
firstRef.current?.focus();
}
};
return (
<>
{focusTrapped.value && <FocusTrap onFocus={onTrapFocus} />}
<div className="wrapper">
<button className="button" ref={firstRef}>
First
</button>
<Checkbox
state={focusTrapped}
clickOnEnter
as="button"
className="button"
>
<CheckboxCheck />
Trap focus
</Checkbox>
<button className="button" ref={lastRef}>
Last
</button>
</div>
{focusTrapped.value && <FocusTrap onFocus={onTrapFocus} />}
</>
);
} |
@diegohaz Wow! Thank you so much 🙇 I've updated the example based on your snippet. I adjusted the styles a bit to make the example a little clearer. Let me know if this works! Happy to make any updates necessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niiice! I made a suggestion to fix the tab order.
Also, would you like to add some tests? We can test with press.Tab()
and press.ShiftTab()
(do a global search on the project to find some examples).
<div className="wrapper"> | ||
<label className="label"> | ||
<Checkbox state={focusTrapped} clickOnEnter className="checkbox" /> | ||
Trap focus | ||
</label> | ||
<button className="button" ref={firstRef}> | ||
First | ||
</button> | ||
<button className="button" ref={lastRef}> | ||
Last | ||
</button> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior is a little bit weird now when the focus is trapped. When pressing Tab, the focus never goes to the checkbox, when pressing Shift+Tab, the focus never goes to the last button.
I think we can just make the checkbox the first item ref to fix this:
<div className="wrapper"> | |
<label className="label"> | |
<Checkbox state={focusTrapped} clickOnEnter className="checkbox" /> | |
Trap focus | |
</label> | |
<button className="button" ref={firstRef}> | |
First | |
</button> | |
<button className="button" ref={lastRef}> | |
Last | |
</button> | |
</div> | |
<div className="wrapper"> | |
<label className="label"> | |
<Checkbox state={focusTrapped} ref={firstRef} className="checkbox" /> | |
Trap focus | |
</label> | |
<button className="button" ref={lastRef}> | |
Button | |
</button> | |
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @diegohaz !! Sorry for the delay! I was able to update the example with your suggestion. I've also added the focus trap test. The Ariakit test utils are really great! ❤️
<div> | ||
<Example /> | ||
<div tabIndex={0}>Outside</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👍 We usually render just <Example />
, but these extra elements are handy here. Do you think we should add another element before so we can test shift+tab as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yea! Let's do that 💪
<div tabIndex={0}>Outside</div> | ||
</div> | ||
); | ||
focus(getByRole("checkbox")); |
There was a problem hiding this 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 use something that better describes a user action here, like await press.Tab()
or await click()
.
); | ||
// Disabling focus trap | ||
await click(getByLabelText("Trap focus")); | ||
await focus(getByRole("button")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await focus(getByRole("button")); | |
await press.Tab(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diegohaz I updated both tests to rely on user interactions - Tab
and ShiftTab
. These utils are great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thank you! ❤️
Thanks a lot for contributing! Based on our community guidelines, every person who has a PR of any kind merged is offered an invitation to the Reakit organization. Should you accept, you'll get write access to the main repository and a fancy Reakit logo on your GitHub profile. You'll be able to label and close issues, create new branches etc. Make sure to read our contribution and community guidelines, which explains all of this in more detail. If you have any questions, let me know! |
This update adds a basic example for the
FocusTrap
component as part of this effort: #939Screenshots
How to test?
yarn dev
Tab
.Tab
focus.Does this PR introduce breaking changes?
Nope! Only adding an example.