-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(TabContainer): Resolve lifecycle deprecation #4370
Conversation
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.
nice!
note the CI junk around lint though \: |
'`generateChildId` prop to TabContainer is required', | ||
); | ||
} | ||
/* eslint-disable react/no-unused-prop-types */ |
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.
@jquense bleh, is there a better way to do this? alternatively, why isn't this triggering for e.g. <Alert>
? does this rule just not work for React.forwardRef
s?
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.
hah, i guess it just doesn't work for ForwardRef
things...
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.
It does work for forwardRef
, just not for prop spread. If a component spreads props into a child, the lint rule passes:
const Alert = _props => <div {...{}} />;
Most forwardRef
components also spread remainder props, so they already trigger this behavior.
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.
ah okay. well, whatever. thanks!
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.
Looks great so far 😁
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, fine by me unless @jquense has a better idea for how to deal with the lint issue
Replace
uncontrollable
with equivalentuseUncontrolled
.Fixes #4368.