As mixin? about react-visibility-sensor HOT 10 CLOSED

fyi: ES6 modules not support mixins today so as a component it is still working with React 0.13 and ES6 class components : https://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html

from react-visibility-sensor.

I'm a little confused, why would how vanilla ES6 works matter for code that is specifically for use with React? Mixings are a core functionality of React, regardless of whether you author it using ES5 or ES6.

from react-visibility-sensor.

Hi @Pomax sorry for the delay replying. I'm not sure I see the advantage here in making this a mixin, even in the example you could do the same thing just as easily by calling setState({ visible }) in the onChange callback.

Having a the VisibilitySensor as a totally separate component can give better separation of concerns, since with mixins you end up with a lot of hidden magical stuff in your component.

Do you have other ideas on benefits of making this a mixin? I'm open to it if there are.

from react-visibility-sensor.

mostly the "I need that magic, instead of having to wrap everything in more and more components". When things are done as components, you end up with stuff like this:

...
render: function() {
  return (
    <Draggable onChange={this.handleDragChange}>
      <VisibilitySensor onChange={this.handleVisibilityChange}>
        <OAuth onChange={this.handleOAuthChange}>
          <...>
            <finally my own stuff>
          </...>
        <//OAuth>
      </VisibilitySensor>
    </Draggable>
  );
},
...

It gets incredibly unwieldy very fast, as opposed to:

module.exports = React.createClass({
  mixins: [
    require("react-draggable"),
    require("react-visibility-sensor"),
    require("react-oauth),
    ...
  ],
  ...
  render: function() {
    return <just my stuff. The mixins already tell me what's tacked on />;
  },
  handleDragChange: function(...) { ... },
  handleVisibilityChange: function(...) { ... },
  handleOAuthChange: function(...) { ... },
  ...
});

from react-visibility-sensor.

Sure, I can see how this gets messy. Out of curiosity is there any reason why VisibilitySensor needs to be wrapping the other components? Could you do your rewrite as:

module.exports = React.createClass({
  mixins: [
    require("react-draggable"),
    require("react-oauth),
    ...
  ],
  ...
  render: function() {
    return (
      <VisibilitySensor onChange={this.handleVisibilityChange} />
      <just my stuff. The mixins already tell me what's tacked on />
    )
  },
  handleDragChange: function(...) { ... },
  handleVisibilityChange: function(...) { ... },
  handleOAuthChange: function(...) { ... },
  ...
});

Or is there a certain behaviour you need that only occurs when there is wrapping / nesting?

from react-visibility-sensor.

Unfortunately React only lets you have a single top level element (since it's actually just syntactic sugar around a JS object creation call), so you'd still have (at the very least):

  render: function() {
    return (
      <div>
        <VisibilitySensor onChange={this.handleVisibilityChange} />
        <just my stuff. The mixins already tell me what's tacked on />
      </div>
    )
  },

from react-visibility-sensor.

Right, forgot about that bit :) Would you consider that a solution? Or still not acceptable?

from react-visibility-sensor.

I suppose that would work, it's less crazy than the nesting at least =)

from react-visibility-sensor.

Cool. Sorry for all the push-back too btw :) It's mostly that I'm just curious to know how to solve these problems without taking on the hidden costs of mixins. That said, I will gladly accept a PR to add mixinification (provided that none of the current functionality is affected).

from react-visibility-sensor.

pushback surfaces the important data, never stop =)

from react-visibility-sensor.