JedWatson / react-tappable

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

Remove useless line (i think) #53

Closed slorber closed 7 years ago

slorber commented 9 years ago
            var newComponentProps = Object.assign({}, props, {
                style: style,
                className: className,
                disabled: props.disabled,
                handlers: this.handlers
            }, this.handlers());

I'm not sure but it seems to me this line handlers: this.handlers is useless right?

JedWatson commented 9 years ago

@nmn this is your code, can you advise?

nmn commented 9 years ago

@JedWatson Hold on... @slorber So, the line that should be removed is: disabled: props.disabled,

That line is useless, as props is already mixed, so disabled is being set twice.

this.handlers looks like this in the TappableMixin:

    handlers: function () {
        return {
            onTouchStart: this.onTouchStart,
            onTouchMove: this.onTouchMove,
            onTouchEnd: this.onTouchEnd,
            onMouseDown: this.onMouseDown,
            onMouseUp: this.onMouseUp,
            onMouseMove: this.onMouseMove,
            onMouseOut: this.onMouseOut
        };
    }

So when mixing the result of this.handlers() the values set will be onMouseDown etc.

handlers is being set to the function itself, not the result. This is for backwards compatibility for people who rely on this.props.handlers in their components.

slorber commented 9 years ago

oh ok I did not know about this backward compatibility issue as I did not know we were able to use this.props.handlers

nmn commented 9 years ago

@slorber react-tappable is very versatile right now. It can be used a component, a HOC and a mixin.

If you use it as a HOC you can set up the event Handlers like so:

render(){
  return (
    <div {...this.props.handlers()} />
  )
}

And

<Tappable component={MyReactComponentClass} onTap={handleTap} />

If you use it as a mixin you would set up like so:

render(){
  return (
    <div {...this.handlers()} />
  )
}

Hope that clears it up a bit.

slorber commented 9 years ago

@nmn yes i've seen that

However I'm not sure Tappable is a HOC can you explain a little bit more please? Because I see it more like a runtime wrapper than a HOC but maybe I'm missing something. See also my answer here: http://stackoverflow.com/a/31564812/82609

pke commented 8 years ago

@nmn could you please clearify what you mean by

If you use it as a HOC you can set up the event Handlers like so:

nmn commented 8 years ago

(@slorber Sorry I didn't reply sooner. Missed it in my github notifications somehow) @pke

I'll try my best to document how react-tappable can be a HOC.

Normal Component

const Tappable = require('react-tappable/Tappable')

...
<Tappable onTap={this.tapHandler}>
  {children}
</Tappable>

In this case, a span will be rendered with all the correct click and touch handlers for you get a reliable tap handler.

Mixin

const TappableMixin = require('react-tappable/TappableMixin')

const CustomEl = React.createClass({
  mixins: [TappableMixin],
  render(){
    //...
    return <div {...this.handlers()}>{children}</div>
  }
})

Here you can extend your own custom component with the tap handler. Using the mixin, you get all the required click and touch handlers by calling this.handlers(). You can just use the spread operator to attach them to your own component.

HOC

const Tappable = require('react-tappable/Tappable')

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

// Usage
<Tappable component={CustomComponent}>{children}</Tappable>

Here, even if you use ES6 classes, you can extend them with onTap behavior. Using the component prop of Tappable, you can pass any React Class as the component to be rendered as the root component for Tappable. Now you could pass div, or some other base element string, and it will work as Tappable will pass down the onClick, onTouchStart etc as individual props too.

But if you want to use a custom component, you will need to bind the click and touch handlers yourself. Just like when you used a mixin. Now, to make this process easier, the handlers method is passed in as a prop.


Let me know if there's still confusion regarding this.

slorber commented 8 years ago

hmmm for me your HOC is not a HOC, it's just a runtime wrapper that can be parametrized with a custom component

nmn commented 8 years ago

@slorber I accept that!