JedWatson / react-tappable

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

Make into a mixin? #6

Closed natew closed 9 years ago

natew commented 9 years ago

This thought crossed my mind the other day. I love the fact that as a component it's incredibly flexible in it's use, but building a UI library for external users it would almost be nice to mix it into every component.

I understand I could use a decorator of sorts, but just wondering your thoughts on this.

Also, I am starting to look into overlapping things. Like what happens if I have a draggable element above a tappable one, sometimes I see the tappable one get the event after the draggable one is finished.

Anyway, just thinking out loud. Wonder your thoughts here.

natew commented 9 years ago

I've started using it here: https://github.com/reapp/reapp-ui/blob/master/mixins/Tappable.js

Reason being, now I can create my own tappable in an external app with different default props, and use that so it keeps those props everywhere. Also am considering mixing it into all the components by default, which I tried without seeing any real performance hit. Problem with that again is it would disallow people from setting default props. Perhaps could have something on init.

JedWatson commented 9 years ago

Seems like a good idea to make the core parts into a mixin, while keeping the ability to use it as a component. How about if the main package exported a Component class, which had a property .Mixin that included pointers to the same methods that would work as a mixin?

So you could do this:

var Tappable = require('react-tappable');

React.render(<Tappable onTap={doSomething} />, el);

// or

React.createClass({
  mixins: [Tappable.Mixin]
});

Not sure if decorating the Class with a Mixin property would cause any issues, would have to look into that, but if not I think this might be a nice way of doing both. What do you think?

natew commented 9 years ago

Yea I think that would work for this case, much like react-tween-state. Especially because it wouldn't add bulk.

I guess in situations where the module is small like this that is preferable to allowing require('react-tappable/mixin'), because it avoids weird requires.

ibirrer commented 9 years ago

See pull request https://github.com/JedWatson/react-tappable/pull/7 for a possible implementation of this.

JedWatson commented 9 years ago

Published. Thanks @ibirrer for the PR!