Forward ref support for TableRow, Tabs, ToggleButton & ToggleButtonGroup #861
Conversation
🦋 Changeset detectedLatest commit: addb5db The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
| id, | ||
| role, |
There was a problem hiding this comment.
This is a bug. It basically provides undefined props and if we don't supply any value explicitly. So if we render a component like this way <View as={RadixTab} /> , both role and id specified internally by radix will be overridden and clear out, probably through {...rest} I guess.
There was a problem hiding this comment.
So, className, role and id will be passed through the {...nonStyleProps} correct?
There was a problem hiding this comment.
Yes, exactly if they are provided. But if we list those out explicitly and not specify values for them, then it will pass in undefined. Equivalent to the following:
{
...
role: undefined
id: undefined
...
}
In most cases, this should be good. But passing in undefined will override underlying components' role and id setting, if they are supposed to be generated internally.
There was a problem hiding this comment.
Yeah, I've seen a related issue with the Radix Dropdown primitives we use in Menu where a passed className prop overrode the internal className and would break the component behavior. It looks like we have a test around id, and role but not for className. Would you mind adding a className test to View.test.tsx?
| onChange, | ||
| indicatorPosition, | ||
| spacing, | ||
| ...rest |
There was a problem hiding this comment.
Thanks for cleaning this up
| const tabs = React.Children.map(children, (child) => { | ||
| if (!isTabsType(child)) { | ||
| console.warn( | ||
| 'Amplify UI: <Tabs> component only accepts <TabItem> as children.' | ||
| ); | ||
| return null; | ||
| return {}; |
There was a problem hiding this comment.
[..., null, ...] will throw error on mapping operation. Cannot deconstruct null or reference to a prop
| return null; | ||
| } | ||
|
|
||
| return React.cloneElement(child, { |
There was a problem hiding this comment.
By cloning, we are able to clone the ref passed along. I cannot find a way to reference ref directly from child
zchenwei
added
pending-review
| indicatorPosition, | ||
| spacing, | ||
| ...rest | ||
| }: TabsProps, |
There was a problem hiding this comment.
| }: TabsProps, | |
| } |
| ref | ||
| ) => ( | ||
| <View | ||
| as={RadixTab} |
There was a problem hiding this comment.
Woah, so cool that this works!
| id, | ||
| role, |
There was a problem hiding this comment.
So, className, role and id will be passed through the {...nonStyleProps} correct?
Co-authored-by: Scott Rees <6165315+reesscot@users.noreply.github.com>
|
This pull request introduces 1 alert when merging aabc200 into b0985f6 - view on LGTM.com new alerts:
|
| @@ -107,6 +107,5 @@ export enum ComponentClassNames { | |||
| TextField = 'amplify-textfield', | |||
| ToggleButton = 'amplify-togglebutton', | |||
| ToggleButtonGroup = 'amplify-togglebuttongroup', | |||
| View = 'amplify-view', | |||
There was a problem hiding this comment.
Looks like this is not in use anywhere? Could we remove it?
Description of changes:
This PR will be adding Forward ref support for
TableRow,Tabs,ToggleButton&ToggleButtonGroupBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.