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
base: master
from

Conversation

Projects
None yet
5 participants
@slorber
Contributor

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?

Remove useless line (i think)
```
			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

This comment has been minimized.

Owner

JedWatson commented Aug 6, 2015

@nmn this is your code, can you advise?

@nmn

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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