Skip to content
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

Remove useless line (i think) #53

Closed
wants to merge 1 commit into from
Closed

Conversation

slorber
Copy link
Contributor

@slorber slorber commented Aug 4, 2015

            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?

```
			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
Copy link
Owner

@nmn this is your code, can you advise?

@nmn
Copy link
Contributor

nmn commented Aug 6, 2015

@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
Copy link
Contributor Author

slorber commented Aug 6, 2015

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
Copy link
Contributor

nmn commented Aug 6, 2015

@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
Copy link
Contributor Author

slorber commented Aug 6, 2015

@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
Copy link

pke commented Apr 29, 2016

@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
Copy link
Contributor

nmn commented Apr 30, 2016

(@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
Copy link
Contributor Author

slorber commented May 1, 2016

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

@nmn
Copy link
Contributor

nmn commented May 1, 2016

@slorber I accept that!

@dcousens dcousens closed this Jan 11, 2017
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.

5 participants