JedWatson / react-tappable

Tappable component for React
http://jedwatson.github.io/react-tappable/
MIT License
862 stars 90 forks source link

make props more flexible #17

Closed nmn closed 9 years ago

nmn commented 9 years ago

This pull request changes the props that are sent to the child component. Instead of only passing down data- and aria- props down, it now passes down ALL props that are not specifically for React-Tappable itself.

The main reason to do this is to make it possible to use custom components with React-Tappable. Currently, similar functionality is possible by using the React-Tappable Mixin, but mixins are not likely to work with ES6 classes in the near future.

This solution makes it possible to use React-Tappble itself, when you would earlier use a mixin.

Example usage:

class CustomComponent extends React.component {
  render(){
    return (
      <div {...this.props.handlers}>
        {everythingElse}
      </div>
    )
  }
}

(Compared to using mixins, this.handlers is replaced with this.props.handlers) Now this custom component can be used elsewhere with React-Tappable like this:

import Tappable from 'react-tappable'
import CustomComponent from './CustomComponent'
<Tappable component={CustomComponent} onTap={this.onTap} customProp='x' customProp2='y' />

This allows you to easily add Tappable powers to any component easily, and without mixins.

Additionally, for simple string based components, (like span, div etc.) The extra props are just ignored by React and are hence harmless.

nmn commented 9 years ago

One more thing:

This pull request is made with the assumption that my other request will be merged. These changes were there in my other pull request, but I broke them out as they are potentially more controversial.

nmn commented 9 years ago

Can you please consider merging this as well?

JedWatson commented 9 years ago

Looks good, would you mind rebasing the PR so I can merge safely + review the changes without the rest of the differences please?

nmn commented 9 years ago

Sadly rebasing this would take more time. I had actually worked on this at the same time as the pinch actions.

I guess, I'll do these changes on the master branch separately by cherry-picking or something.

nmn commented 9 years ago

Hey! I re-did the changes so you can merge easily. Apart from the big benefit of using React-Tappable as a wrapper component, there is another important use-case for flexible props: Links

In yarr.js, I wrap every Link in a Tappable component.

These links are just anchor tags. But because of Tappable, they don't get the href property.

For example:

<Tappable component='a' onTap={this.openLink} href="https://google.com" target="_blank" />

This code now only works with javascript, and the anchor link doesn't actually get the href and target properties. Flexible props would make this work correctly.

JedWatson commented 9 years ago

This is great, thanks a lot for the update @nmn!