-
Notifications
You must be signed in to change notification settings - Fork 71
[LG-5608] feat(modal): restore initialFocus prop with enhanced functionality #3202
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
base: main
Are you sure you want to change the base?
Conversation
…nstead adds guidance comment
🦋 Changeset detectedLatest commit: f6058df The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
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.
Pull Request Overview
This PR restores the initialFocus
prop to the Modal component that was prematurely removed in v20. The prop now supports enhanced functionality including React refs, CSS selectors, automatic behavior, and the ability to disable focus management entirely.
Key changes:
- Restores
initialFocus
prop with support for"auto"
(default), string selectors, React refs, andnull
- Updates focus priority:
initialFocus
prop →autoFocus
attribute → first focusable element - Updates the
modal-v20
codemod to preserve theinitialFocus
prop instead of removing it
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tools/codemods/src/codemods/modal-v20/transform.ts |
Updates codemod to preserve initialFocus prop and add guidance comment instead of removing it |
tools/codemods/src/codemods/modal-v20/tests/transform.spec.ts |
Removes test for remove-initialFocus behavior that is no longer applicable |
tools/codemods/src/codemods/modal-v20/tests/remove-initialFocus.output.jsx |
Deletes expected output file for removed initialFocus test |
tools/codemods/src/codemods/modal-v20/tests/remove-initialFocus.input.jsx |
Deletes input file for removed initialFocus test |
tools/codemods/src/codemods/modal-v20/tests/handle-aliases.output.jsx |
Updates expected output to preserve initialFocus props with new guidance comment |
tools/codemods/src/codemods/modal-v20/tests/filter-packages.output.jsx |
Updates expected output to preserve initialFocus props with new guidance comment |
tools/codemods/README.md |
Updates codemod documentation to reflect that initialFocus is now preserved |
packages/modal/src/utils/focusModalChildElement.ts |
Adds support for the restored initialFocus parameter with enhanced functionality |
packages/modal/src/utils/focusModalChildElement.spec.ts |
Adds comprehensive tests for all initialFocus scenarios and priority ordering |
packages/modal/src/Modal/ModalView.tsx |
Integrates initialFocus prop and passes it to focus utility function |
packages/modal/src/Modal/Modal.types.ts |
Adds type definition and comprehensive documentation for initialFocus prop |
packages/modal/src/Modal/Modal.spec.tsx |
Adds extensive test coverage for all initialFocus behaviors and edge cases |
packages/modal/UPGRADE.md |
Updates upgrade guide to reflect that initialFocus is available again in v20.2+ |
packages/modal/README.md |
Updates documentation with initialFocus prop details and focus behavior explanation |
packages/modal/CHANGELOG.md |
Updates changelog to clarify initialFocus removal and restoration timeline |
.changeset/modal-initial-focus-revert.md |
Adds changeset documenting the restoration of initialFocus prop |
.changeset/codemods-modal-v20-update.md |
Adds changeset documenting codemod updates |
Size Change: +70 B (0%) Total Size: 1.59 MB
ℹ️ View Unchanged
|
* 4. If `initialFocus` is `null`, no automatic focus occurs | ||
* | ||
* @param modalElement - The modal dialog element | ||
* @param initialFocus - Focus target ("auto", selector, ref, or null) |
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.
My initial expectation would be to pass false
instead of null
here to disable the initial focus behavior 🤔
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 could be down for false
over null
. It was also my natural inclination but was worried it could be confusing since true
is not a valid value. I will solicit feedback with the team on this as well. Appreciate your feedback
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.
Lots of great stuff! Thanks for the fast turnaround on this!
Did you consider initialFocus={false}
instead of initialFocus={null}
to signal disabling the auto-focus behavior all together? I don't feel strongly for either way, so not a blocker.
I personally feel the focusModalChildElement
tests are getting a bit lengthy and hard to follow with the way the mocks are used. This is definitely not a blocker.
✍️ Proposed changes
This PR restores the
initialFocus
prop to the Modal component that was prematurely removed in v20 without proper migration paths. The prop now supports enhanced functionality including React refs, CSS selectors (legacy support), an"auto"
default behavior, and the ability to disable focus management withnull
.Key changes:
initialFocus
prop with support for"auto"
, string selectors, React refs, andnull
initialFocus
prop →autoFocus
attribute → first focusable elementmodal-v20
codemod to preserveinitialFocus
and add migration guidance🎟️ Jira ticket: LG-5608
✅ Checklist
pnpm changeset
and documented my changes🧪 How to test changes
Test auto focus (default behavior):
initialFocus
Test focus with string selector:
initialFocus="#my-button"
on a Modalid="my-button"
receives focusTest focus with ref:
useRef<HTMLButtonElement>(null)
initialFocus={myRef}
Test disabled focus:
initialFocus={null}
on a ModalTest priority order:
initialFocus
prop andautoFocus
attribute on different elementsinitialFocus
takes priority overautoFocus
Test fallback behavior:
initialFocus
to a non-existent selectorautoFocus
attribute, then first focusable element