RadioControl: add ref forwarding#41641
Conversation
| * }; | ||
| * ``` | ||
| */ | ||
| export const RadioControl = forwardRef( UnforwardedRadioControl ); |
There was a problem hiding this comment.
@ciampo Does that function name seem confusing? UnforwardedRadioControl? Not sure if other components use that but it just seems weird especially reading from start to finish as the function now forwards the ref.
There was a problem hiding this comment.
This is a pattern that @mirka and I have adopted in the context of the TypeScript migration that we're undertaking across the components in the package (see this guide for more details)
In order to have Storybook extract and show prop types correctly, we need to have a named export for the component ("RadioControl" in this case). Therefore, we had to find a suitable name for the intermediate representation of the component, and we opted for:
- adding the "unforwarded" prefix when the component is later wrapped in a
forwardRefcall - adding the "unconnected" prefix when the component is later wrapped in a
contextConnectcall
Hope that's a bit more clear!
walbo
left a comment
There was a problem hiding this comment.
One question. Other than that everything looks good to me
| aria-describedby={ | ||
| !! help ? `${ id }__help` : undefined | ||
| } | ||
| ref={ forwardedRef } |
There was a problem hiding this comment.
Havn't used forwardRef inside an Array.map before. Anything we need to think about or is this pattern ok?
There was a problem hiding this comment.
Good point — I had actually missed that map statement while making these changes quickly.
It turns out that this task is more complicated than expected — we could forward the ref to BaseControl, but BaseControl currently doesn't accept refs.
I tried to quickly add ref forwarding to BaseControl, but I ran into type issues — mainly to do with the fact that VisualLabel is currently defined as a static property on BaseControl (e.g. BaseControl.VisualLabel), and adding ref forwarding causes an error with that
Since this is not currently a priority, I've decided to stop investigating this further for now. What do folks think is the best way forward? I'm also happy to drop this entirely for the time being.
There was a problem hiding this comment.
I agree we can drop it for now, unless we have a concrete use case. I came across this note in the React docs that forwarding refs is a breaking change, and while I'm not 100% sure about what that entails, it did give me pause.
There was a problem hiding this comment.
Makes sense. Will close this PR for now
What?
Adds ref forwarding to the
RadioControlcomponentWhy?
Consistency with other components in the package (both in terms of ref forwarding, and in terms of code patterns specifically around how we export a component).
How?
Wrapped the component into
forwardRef, renamed original component toUnforwardedRadioControl, moved JSDocsTesting Instructions