Export useFocusTrap hook - #1420
Conversation
Copilot-Session: b43bee82-7a2d-40d0-95c2-9b053bdc6075
🦋 Changeset detectedLatest commit: bbb6f4b The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
There was a problem hiding this comment.
Pull request overview
This PR makes the useFocusTrap hook part of the public @primer/react-brand API by exporting it from the package root, and updates the hook to always provide stable containerRef/initialFocusRef values via useProvidedRefOrCreate when refs are not supplied.
Changes:
- Exported
useFocusTrapfrompackages/react/src/index.ts. - Updated
useFocusTrapto accept optional refs and to create stable fallback refs when not provided. - Added a changeset documenting the new export + behavior.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/index.ts | Exports useFocusTrap from the package root. |
| packages/react/src/hooks/useFocusTrap.ts | Uses useProvidedRefOrCreate to ensure stable refs and updates types accordingly. |
| .changeset/tidy-traps-focus.md | Declares a minor release and documents the new export + ref behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Low
🟢 Unit test coverage changes foundUnit test coverage has been updated through this PR. Changes: 1 new tests, 0 removed tests, 0 improved, 0 decreased
|
🟢 Bundle size report
|
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
Copilot-Session: b43bee82-7a2d-40d0-95c2-9b053bdc6075
Copilot-Session: b43bee82-7a2d-40d0-95c2-9b053bdc6075
Copilot-Session: b43bee82-7a2d-40d0-95c2-9b053bdc6075
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
packages/react/src/hooks/useFocusTrap.ts:60
restoreFocusOnCleanUpcurrently only restores focus when the previously focused element is anHTMLElement.document.activeElementcan be a non-HTMLElement(e.g. a focusable SVG element withtabindex), so focus restoration will silently no-op even though the setting says it restores focus to the element that was focused before activation. Consider restoring focus for any element that exposes afocus()method instead of restricting toHTMLElementonly.
if (settings?.restoreFocusOnCleanUp && previousFocusedElement.current instanceof HTMLElement) {
previousFocusedElement.current.focus()
}
previousFocusedElement.current = null
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Low
There was a problem hiding this comment.
Does this need to be a separate file @danielguillan? Can we add the test case to the useFocusTrap.test.tsx file?
There was a problem hiding this comment.
I believe so. My understanding is that Jest’s node environment is file-scoped.
rezrah
left a comment
There was a problem hiding this comment.
LGTM. Thanks for fixing this Dani.
Copilot-Session: 28a6bc72-61a7-4142-933a-df469ee7461e
Copilot-Session: 28a6bc72-61a7-4142-933a-df469ee7461e
Summary
Resolves #1399
Exports
useFocusTrapfrom the@primer/react-brandpackage.The hook now creates missing refs automatically and captures previous focus when the trap activates.
List of notable changes:
useFocusTrapandFocusTrapHookSettingsfrom the package rootuseFocusTrapto create missing refs while preserving supplied object refsuseEffectso rendering remains SSR-safeSupporting resources (related issues, external links, etc):
Contributor checklist:
update snapshotslabel to the PR)Reviewer checklist:
Screenshots:
N/A