Add a useFocusOnMount hook to the @wordpress/compose package. #27574
Conversation
youknowriad
added
the
[Type] New API
youknowriad
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +186 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
|
||
| - Type: `Function` | ||
|
|
||
| A function reference that must be passed to the DOM element where constrained tabbing should be enabled. |
youknowriad
changed the title
|
Is there any other use case for this hook right now, since we're exporting this API? I see some uncertainties around the ref, so maybe it's too early to export/stabilise? |
|
I've used the hook in more places in a follow-up. In all PopoverWrapper components. What's the uncertainty here? |
|
|
||
| ### `ref` | ||
|
|
||
| - Type: `Function` |
There was a problem hiding this comment.
So this is incorrect, right? An object ref is returned.
There was a problem hiding this comment.
Oh right! I guess it should just say "Element ref". We don't really care whether it's a function or object.
|
Ah ok 👍
The kind of ref returned, which is a minor thing I guess. Still, it could be a breaking change. |
That's a good point, though, for me, it should probably reference the React Ref Element type. Users of the hook shouldn't be assuming what kind of ref it is. |
|
I'll do a follow-up to type these hooks properly. |
|
Right, they shouldn't, but sometimes you're using the current property assuming the ref will always be an object. We have some code like that. :) I sometimes think it could be useful to create refs that are both, meaning a function which also has a getter named current. |
| const focusOnMountRef = useRef( focusOnMount ); | ||
| useEffect( () => { | ||
| focusOnMountRef.current = focusOnMount; | ||
| }, [ focusOnMount ] ); |
There was a problem hiding this comment.
Since focusOnMount is only used on mount, there doesn't really seem to be a need for updating it?
There was a problem hiding this comment.
As implemented here yes but I updated this hook later to solve the issue where the "ref" changes. an element can unmount without the component unmounting.
|
I'm still not sure if it's a good idea to stabilise this hook. Components should autofocus themselves, the parent component shouldn't do that. That's why the timeout is needed I think. Unless this problem can be solved, there remains some uncertainty around this API in my opinion. I think we should pass a prop down to the component that need autofocusing. |
For me no but I'd love to hear more about why you think that.
For me, it's far less than ideal and breaks composability. |
|
I explored this in #27698. The first item doesn't necessarily need receive focus, it could be any element. When it's done by the |
|
Ok, I agree with you. Let's keep the current behaviour. Still trying to get rid of the strange timeout in #27699. |
|
yes, I didn't give the timeout much thought in my refactoring, I just kept it from the previous implementation. Maybe it's not useful anymore. |
Follow-up to #27502
Similar to #27544
This extracts the existing useFocusOnMount hook from the popover component into the compose package in order to be used in the existing PopoverWrapper components.
The goal here is to be able to remove the PopoverWrapper component that is duplicated across three packages and reuse a single hook.