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

Revamp API to cleanly separate configuration and effects #74

Open
neeharv opened this issue Apr 12, 2017 · 3 comments
Open

Revamp API to cleanly separate configuration and effects #74

neeharv opened this issue Apr 12, 2017 · 3 comments

Comments

@neeharv
Copy link
Collaborator

neeharv commented Apr 12, 2017

This is a more philosophical question about the API of this lib

Currently, the component depends on its effects propagating upwards by a lifecycle-ish onChange method, which usually triggers a change in state of the parent component / update store etc. However, this approach merges together the configuration of the component (scrollDelay, intervalDelay etc), with its effects (onChange), which makes it slightly confusing to grok IMO.

Inspired by React Router v4, I propose the following API

const MyVisibilitySensor = createSensor({
  delayInterval : 200,
  // other opts
});

const App = () => (
  <MyVisibilitySensor>
    {
      ({ isVisible }) => (
        isVisible ? <MainImage /> : <PlaceholderImage />
      )
    }
  </MyVisibilitySensor>
);

This separates concerns very nicely, and also avoids the overhead of having a situation where we have a hierarchy of parent component -> visibility sensor, and the onChange callback triggers a re-render from the parent component downwards. It helps avoid one layer of lifting state up, and is easier to read IMO.

Thoughts?

@Andarist
Copy link
Contributor

How would u use this API with onChange? I specifically mean the combo of onChange + render callback situation

@neeharv
Copy link
Collaborator Author

neeharv commented Apr 20, 2017

How would u use this API with onChange? I specifically mean the combo of onChange + render callback situation

Not sure of the question. The render callback is the onChange method now. Every time a visibility change happens, the render callback is executed.

@joshwnj
Copy link
Owner

joshwnj commented Jun 6, 2017

@neeharv I really like the direction this is going, thanks for kicking off the discussion :)

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

No branches or pull requests

3 participants