fix(react): fix issue where ref property was getting clobberd on <Tab> #728
Conversation
|
Preview branch generated at https://tab-ref.d1gko6en628vir.amplifyapp.com |
| @@ -122,7 +122,6 @@ const Tabs = ({ | |||
| tabIndex: index === activeIndex ? 0 : -1, | |||
| ['aria-controls']: target.current?.id, | |||
| ['aria-selected']: selected, | |||
| ref: index === activeIndex ? focusedTabRef : null, | |||
There was a problem hiding this comment.
This was clobbering the ref, in reality we don't really need it when we can utilize [aria-selected="true"]
There was a problem hiding this comment.
I believe what you have implemented here should work. However, I have found relying on query selectors in React components to be somewhat finicky. For example, sometimes the DOM isn't in the state you expect it to be in. Or, the structure changes and your query selectors are now incorrect.
fwiw, it should be possible to get this working closer to the way it was originally implemented. See: facebook/react#8873 (comment) and facebook/react#8873 (comment).
| ref: index === activeIndex ? focusedTabRef : null, | |
| ref: index === activeIndex ? (node) => { | |
| focusedTabRef.current = node; | |
| if (typeof child.ref === 'function') { | |
| child.ref(node) | |
| } else if (child.ref !== null) { | |
| child.ref.current = node | |
| } | |
| } : child.ref |
There was a problem hiding this comment.
The query selector shouldn't need to change unless something were to change with the WCAG spec respective of how tabs are implemented. I feel that it's much safer to rely on query here rather than trying to patch into whatever React uses for refs.
| @@ -122,7 +122,6 @@ const Tabs = ({ | |||
| tabIndex: index === activeIndex ? 0 : -1, | |||
| ['aria-controls']: target.current?.id, | |||
| ['aria-selected']: selected, | |||
| ref: index === activeIndex ? focusedTabRef : null, | |||
There was a problem hiding this comment.
I believe what you have implemented here should work. However, I have found relying on query selectors in React components to be somewhat finicky. For example, sometimes the DOM isn't in the state you expect it to be in. Or, the structure changes and your query selectors are now incorrect.
fwiw, it should be possible to get this working closer to the way it was originally implemented. See: facebook/react#8873 (comment) and facebook/react#8873 (comment).
| ref: index === activeIndex ? focusedTabRef : null, | |
| ref: index === activeIndex ? (node) => { | |
| focusedTabRef.current = node; | |
| if (typeof child.ref === 'function') { | |
| child.ref(node) | |
| } else if (child.ref !== null) { | |
| child.ref.current = node | |
| } | |
| } : child.ref |
|
Preview branch generated at https://tab-ref.d1gko6en628vir.amplifyapp.com |
This fixes an issue where
<Tab ref={tabRef}>was alwaysnullbecause it was getting clobbered withincloneElement.