allow passing a children function that takes state and chooses what to render from it #76

jedwards1211 · 2017-04-20T18:14:09Z

This is the number one feature I've wanted in react-visibility-sensor for a long time. It's a pattern I first saw in react-motion that I've really come to like. I also got the creator of react-measure to add this pattern to his package. Here's what it looks like:

  <VisibilitySensor>
    {({isVisible}) =>
      <div>I am {isVisible ? 'visible' : 'invisible'}</div>
    }
  </VisibilitySensor>

This is super convenient if all you need to do is change what you render inside the component based upon whether it's visible, because you don't have to go to the trouble of storing the visibility state anywhere in your own code.

This PR doesn't cause any breaking changes; you can still use a React element for the children, and a children function can be used together with onChange or without it -- I made onChange optional.

neeharv · 2017-04-20T18:28:01Z

Nice work! We've been discussing this over at #74

neeharv · 2017-04-20T18:29:22Z

visibility-sensor.js

    }

    return state;
  },

  render: function () {
+    if (this.props.children instanceof Function) return this.props.children(this.state);


I would destructure this.state here to return explicit values. Prevents leaking of internal state details to the render callback in the future.

@neeharv agreed.

@jedwards1211 what do you think?

@neeharv @joshwnj yeah, makes sense

I updated it

neeharv · 2017-04-20T18:30:52Z

README.md

@@ -66,6 +77,7 @@ Props
 - `resizeThrottle`: (default: `-1`) by specifying a value > -1, you are enabling throttle instead of the delay to trigger checks on resize event. Throttle supercedes delay.
 - `containment`: (optional) element to use as a viewport when checking visibility. Default behaviour is to use the browser window as viewport.
 - `delayedCall`: (default `false`) if is set to true, wont execute on page load ( prevents react apps triggering elements as visible before styles are loaded )
+- `children`: can be a React element or a function.  If you provide a function, it will be called with 1 argument `{isVisible: ?boolean, visibilityRect: Object}`


What's the use case for passing visibilityRect to the render callback?

I guess it's useful for determining partial visibility

@neeharv why not? Libs like react-virtualized only render subcomponents that are within their own visibility rectangle, so I figured having visibilityRect in the render callback could be useful.

joshwnj · 2017-06-06T03:23:13Z

Thanks @jedwards1211 I really like the proposal here - thanks for your patience while I found some time to review it :) Looks great and I like the simplicity it introduces.

@neeharv anything else you think we ought to do here before merging?

neeharv · 2017-06-06T06:49:03Z

package.json

@@ -36,8 +36,8 @@
    "karma-mocha": "^0.1.9",
    "karma-phantomjs-launcher": "^0.1.4",
    "mocha": "^1.21.4",
-    "react": "^0.14.0 || ^15.0.0",
-    "react-dom": "^0.14.0 || ^15.0.0",
+    "react": "^15.5.4",


Do we need to update deps in this PR? Unsure of knock on effects. Maybe best left to keep the scope of this to just the code change.

I made that change because I don't think it's really appropriate to || multiple versions together in devDependencies. I only do that in peerDependencies. But it's true, we could leave this change out of this PR.

okay, reverted

jedwards1211 · 2017-06-06T18:23:45Z

Hey guys, the build errored saying that it couldn't find uglifyjs. I'm confused because I don't see how that could have changed. Do you know why that would be?

joshwnj · 2017-06-07T00:04:12Z

@jedwards1211 I think it's due to the deprecation warning shown here: #80

Please try rebasing and then we should be good to merge.

jedwards1211 · 2017-06-07T00:08:46Z

@joshwnj oh. You might want to try using yarn for the CI build so you can have a lockfile; then you would only see build failures like this when you explicitly choose to upgrade a dependency (or have something like Greenkeeper automatically PR dep uprades)

(and documentation)

jedwards1211 · 2017-06-07T00:24:26Z

Okay, this should be ready!

joshwnj · 2017-06-07T00:24:49Z

something like Greenkeeper automatically PR dep uprades

@jedwards1211 good call. There's a rewrite in motion, so I think we'll hold off adding greenkeeper until then.

joshwnj · 2017-06-07T00:28:03Z

Published to npm v3.10.0.

Thanks for your work on this @jedwards1211 ! And @neeharv for the reviews!

jedwards1211 · 2017-06-07T00:35:30Z

Sweet, wow, that was fast!

neeharv requested changes Apr 20, 2017

View reviewed changes

neeharv reviewed Jun 6, 2017

View reviewed changes

jedwards1211 added 4 commits June 6, 2017 19:14

feat(visibility-sensor.js): support child function

0ab021c

test(children-function): add children function test

5665b9d

(and documentation)

refactor(VisibilitySensor): don't pass this.state to child function

97d5a57

fix(package.json): revert devDependencies changes

9b8c306

jedwards1211 force-pushed the children-function branch from 74eb48e to 9b8c306 Compare June 7, 2017 00:19

joshwnj merged commit 9c11db6 into joshwnj:master Jun 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow passing a children function that takes state and chooses what to render from it #76

allow passing a children function that takes state and chooses what to render from it #76

jedwards1211 commented Apr 20, 2017 •

edited

Loading

neeharv commented Apr 20, 2017

neeharv Apr 20, 2017

joshwnj Jun 6, 2017

jedwards1211 Jun 6, 2017

jedwards1211 Jun 6, 2017

neeharv Apr 20, 2017

joshwnj Jun 6, 2017

jedwards1211 Jun 6, 2017

joshwnj commented Jun 6, 2017

neeharv Jun 6, 2017

jedwards1211 Jun 6, 2017

jedwards1211 Jun 6, 2017

jedwards1211 commented Jun 6, 2017

joshwnj commented Jun 7, 2017

jedwards1211 commented Jun 7, 2017

jedwards1211 commented Jun 7, 2017

joshwnj commented Jun 7, 2017

joshwnj commented Jun 7, 2017

jedwards1211 commented Jun 7, 2017

allow passing a children function that takes state and chooses what to render from it #76

allow passing a children function that takes state and chooses what to render from it #76

Conversation

jedwards1211 commented Apr 20, 2017 • edited Loading

neeharv commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwnj commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedwards1211 commented Jun 6, 2017

joshwnj commented Jun 7, 2017

jedwards1211 commented Jun 7, 2017

jedwards1211 commented Jun 7, 2017

joshwnj commented Jun 7, 2017

joshwnj commented Jun 7, 2017

jedwards1211 commented Jun 7, 2017

jedwards1211 commented Apr 20, 2017 •

edited

Loading