-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(useFocusTrap): expose updateContainerElements for dynamic contai… #4849
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?
feat(useFocusTrap): expose updateContainerElements for dynamic contai… #4849
Conversation
const updateContainerElements = (el: ContainerElements) => { | ||
if (trap) { | ||
trap.updateContainerElements(el) | ||
return trap | ||
} | ||
} |
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 dont want to expose this, but watch the target and updateContaienrElements
when target changes (instead of activate/deactivate)? wdyt @ilyaliao
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 dont want to expose this, but watch the target and
updateContaienrElements
when target changes (instead of activate/deactivate)? wdyt @ilyaliao
Yes, that's what I initially had in mind as well. We could initialize once with createFocusTrap
, and then update the element via updateContainerElements
afterwards. But I haven’t looked into the feasibility of this approach in depth yet.
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.
@PeikyLiu wdyt?
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.
ok,I'll try based your idea,but it will be take some time
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 dont want to expose this, but watch the target and
updateContaienrElements
when target changes (instead of activate/deactivate)? wdyt @ilyaliao
To summarize: you suggest using watch to monitor target changes inside the hook, while keeping the activate/deactivate methods exposed, right?
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.
we always want to use the target thats passed to the composable. If we want to change the target we want to update the target ref. So we can use updateContainerElements, we should do this while watching target 😄
🚀 Feature: Expose updateContainerElements method in useFocusTrap
Description
This PR exposes the
updateContainerElements
method from the underlying focus-trap library, allowing users to dynamically update focus trap containers while the trap is active.Related Issues
resolve #4848
📋 Changes
updateContainerElements
method toUseFocusTrapReturn
interfaceContainerElements
type alias usingParameters<FocusTrap['updateContainerElements']>[0]
undefined
case when trap is not initialized🔧 Technical Details
Parameters
utility type to automatically inherit parameter types from focus-trap library📚 Example Usage
🔄 Breaking Changes
None - this is a purely additive change.