-
Notifications
You must be signed in to change notification settings - Fork 172
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
rename isOpened
prop to isOpen
, add deprecation notice
#82
Conversation
The deprecation notice should be produced by the library. |
Adding a HOC to transform the props breaks almost all tests. Do you have a suggestion for providing |
+1 would love to see this merged! |
if (typeof props[propName] !== 'undefined') { | ||
return new Error( | ||
`Deprecated prop \`${propName}\` supplied to \`${componentName}\`. | ||
Please use \`isOpen\` in the future.` |
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.
"In the future" might be misleading here - this is an error, you have to use isOpen
for the portal to work, right?
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.
Yep. I'm going to change the message.
Merged. Thanks! |
As a point of discussion (I can open up another issue if you like), why does this prop even exist? It makes more sense to me to simply always render the portal if it's in the tree, and to just not render it if you want to hide it. Like so: return (
<div>
<button onClick={() => this.setState({opened: !this.state.opened})}>Toggle</button>
{this.state.opened ?
<Portal><div>Portal Contents</div></Portal>
: null}
</div>
); |
I like the idea of not using
I guess I thought it's more intuitive. |
fixes #61