Skip to content

Conversation

@hugofelp
Copy link

@hugofelp hugofelp commented Aug 3, 2016

  • included local copy of eslint to enable consistent testing
  • made onClose callback optional
  • filtered component own properties from div to avoid "Unknown prop" warning Unknown prop warning #14

If you're happy with the changes, would you care to update the package on npm?
Thanks a lot.

@zackify
Copy link
Member

zackify commented Aug 3, 2016

Thanks a lot for the pull request and adding es lint! I've been keeping up with the latest ideas on fixing the prop warning. I think we should go ahead and accept a new prop, domProps. And just spread ...this.props.domProps. That would mean no filtering. But also a major version change. Anything not specific to the component would be passed in as an object to domProps. What do you think? Dan Abramov suggested this idea on twitter. If you like it, add it and I'll add a new release ASAP.

@hugofelp
Copy link
Author

hugofelp commented Aug 3, 2016

That is indeed a good idea, but as you pointed out it will be a breaking change, potentially breaking apps that inadvertently update the dependency. What if we kept the filter as it is, for backwards compatibility, and also included this new property as a the recommended way of passing DOM props? You could then drop support to the old filter on a future major release...

@hugofelp
Copy link
Author

hugofelp commented Aug 3, 2016

Or, just save this idea of a new prop altogether for a future major release? thoughts?

@zackify
Copy link
Member

zackify commented Aug 3, 2016

Hmm, I guess I will go ahead and merge it. Then will make a new major
release with the idea I have. Instead of just making a new major right
away.
On Wed, Aug 3, 2016 at 00:43 Hugo notifications@github.com wrote:

Or, just save this idea for altogether for a future major release?
thoughts?


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#15 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAbacANRADHfNgmTHq1GafpMCMc97Vgbks5qcBxXgaJpZM4JbOq6
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants