Accept children #152

rattrayalex · 2017-01-25T17:46:39Z

Addresses #112
Intended to supersede #41

If you pass children to a Waypoint element, onEnter will be triggered when the element is fully or partially within the viewport, or if it fully encloses the viewpoint. onLeave will be triggered when no part of the element is within the viewport.

This does not differentiate between full, partial, or enclosing visibility. That would likely require API changes (eg; a parameter being passed to onEnter and onLeave, new callbacks like onPartialEnter, or a new callback with a parameter, eg onChange(visibility: Enum<'full'|'partial'|'enclosing'|'none'>). Leaving as a TODO.

I haven't added tests yet, so this isn't ready to merge. In the meantime, though, here's a screencast:

rattrayalex · 2017-01-25T20:03:40Z

@jmeas I could use some help/direction with tests. I copy/pasted several of the main tests that seemed like they could use duplication with children provided, but my hunch is this is largely a waste.

Instead, one or two tests ensuring that fully-enclosing and top-above, bottom-visible work should be sufficient, but I struggled to write those effectively. Any chance you could sketch out a skeleton for those?

trotzig

I agree about with the lighter testing approach. Let me see if I can carve out a test.

trotzig · 2017-01-26T08:58:07Z

src/waypoint.jsx

-    return <span ref={this.refElement} style={{ fontSize: 0 }} />;
+    if (this.props.children) {
+      // TODO: consider returning children directly when there is only one child,
+      // using React.cloneElement to attach a ref (but child might already have a ref)


I think the wrapping div might be just fine, though I understand it if we want to limit the number of DOM nodes.

It's also a tad unfortunate to have to inject an element which may interfere with styling. But I can remove the TODO because it's unlikely to be worth it

I understand the desire to keep the number of nodes small. I wonder if returning children directly would require a refactoring of _getBounds to account for an array of children, as we wouldn't be able to rely on a single element's getBoundingClientRect?

In any event, I agree with @trotzig that this is good as-is 👍

😄 awesome!

@jmeas I don't think that's allowed in any case – facebook/react#2127. IIRC, children is either an array or a single element (not wrapped in an array), so when it's a single element, you can return it directly (after adding a ref attribute). But that's a lot of work for a potentially limited usecase

You're right, @rattrayalex – it's not currently possible. I'm thinking about the future when Fiber supports it. The current code prepares us better for that day.

Wow, looks like Fiber may be close to shipping! Exciting!

trotzig · 2017-01-26T08:59:19Z

src/waypoint.jsx

@@ -295,8 +311,9 @@ export default class Waypoint extends React.Component {

  _getBounds() {
    const horizontal = this.props.horizontal;
-    const waypointTop = horizontal ? this._ref.getBoundingClientRect().left :
-      this._ref.getBoundingClientRect().top;
+    const rect = this._ref.getBoundingClientRect();


nit: you could deconstruct this:

const { left, top, right, bottom } = this._ref.getBoundingClientRect();

trotzig · 2017-01-26T09:00:06Z

src/waypoint.jsx

+  }
+
+  // TODO: consider returning more granular values than 'inside',
+  // eg; `partiallyInside`, `fullyInside`, and `fullyEnclosing`


I don't know your exact use-case, but what you have seems perfectly fine to start with.

trotzig · 2017-01-26T09:53:32Z

I noticed you found a way to get the specs working. I agree that they're not easy to use/update, I'm to blame for that. There's too much indirection, and I think the tests could benefit from a refactor where we would break it up into smaller pieces, and preferably avoided most of the indirection.

That being said, I hacked together an example of tests I think would be sufficient for this change: https://gist.github.com/trotzig/4321f7846c35039563411b94498a7d5c

The first test is nested under when the Waypoint is below the bottom. The second one couldn't be nested so I duplicated some stuff. Feel free to use this or just stick with what you have.

Thanks for contributing!

rattrayalex · 2017-01-26T17:02:16Z

Awesome, thanks for the starting point @trotzig ! I'll get cracking on that shortly.

rattrayalex · 2017-01-26T18:46:20Z

Awesome, updated! @trotzig let me know if this looks good and I'll squash the commits

jamesplease · 2017-01-26T20:06:34Z

src/waypoint.jsx

+    return POSITIONS.inside;
+  }
+
+  // top is above the screen and bottom is below the screen


I think this issue goes beyond this PR, but we should be consistent about the name that we give the "containing element that is scrollable." Here it's being called "screen," the prop to manually set it it is "scrollableAncestor," and when the offsets are taken into account, its called the "viewport." Does anyone else think this could affect someone's understanding of the code? If not, then maybe it's no big deal.

[oops, responded to wrong comment – deleted previous message]

I agree, there is value in being consistent with terminology. It would be nice if this PR didn't make the terminology more cloudy at the least.

jamesplease · 2017-01-26T20:17:33Z

src/waypoint.jsx

+
+  // top is above the screen and bottom is below the screen
+  if (bounds.waypointTop <= bounds.viewportTop &&
+      bounds.viewportBottom <= bounds.waypointBottom) {


Doesn't matter too much, but we could get away with these just be <, I think. The == case should be handled by the above two conditionals.

jamesplease · 2017-01-26T20:20:02Z

src/waypoint.jsx

+      bounds.viewportBottom <= bounds.waypointBottom) {
+    return POSITIONS.inside;
+  }
+
  if (bounds.viewportBottom < bounds.waypointTop) {
    return POSITIONS.below;
  }


It may make sense to make the "outside" conditionals first. I suspect on a page with lots of waypoints, most of them will not be "inside," so this would allow us to skip over the other conditions. This may just be a micro-optimization with no real performance gain, though. What do y'all think?

Hmm... I'm slightly concerned about the possibility of bug introduction, and the above conditionals do not seem likely to be expensive operations, so I'd lean towards leaving as-is personally.

jamesplease · 2017-01-26T20:22:55Z

This is really good. Thanks for putting this together, @rattrayalex ! I left a few comments, though none of them are blockers. They're just a few thoughts I had while reading the code.

We'll also want to update the README so that folks know they can pass children. The guides on boundaries and offsets may need to be updated, too, to account for the fact that waypoints can now have height. I can grab some of that / all of that if you want @rattrayalex . Just lmk.

Also, I haven't checked out the tests yet...about to do that now.

Update: tests look good to me.

rattrayalex · 2017-01-26T20:29:43Z

Thanks @jmeas !

Would be awesome if you could update docs, as I'm stressing my "open source" budget pretty thin for the week 😉

jamesplease · 2017-01-26T20:43:57Z

Sure thing. I'll update the docs today or tomorrow.

rattrayalex · 2017-01-26T21:35:09Z

Sweet! I've squashed my commits; feel free to merge (or modify & merge) when you like!

jamesplease · 2017-01-28T06:38:48Z

Thanks for squashing @rattrayalex . I'll add an extra commit into this PR to update the docs tomorrow; didn't have 🕒 today.

jamesplease · 2017-01-29T05:19:47Z

Alright, sorry for the delay, y'all! I whipped up some quick docs, and committed it to this branch. Let me know if you think any of it should be changed / rearranged / moved / deleted / whatevs.

Also, I'm totally cool if we want to squash the docs commit down to @rattrayalex 's original commit.

lencioni · 2017-01-29T15:59:47Z

README.md

+Waypoints can have children, allowing you to track when a section of content
+enters or leaves the viewport.
+
+```javascript


nit: This should be jsx, not javascript (I see that this is incorrect elsewhere in this file as well)

Cool, i'll update all of these.

lencioni · 2017-01-29T16:01:54Z

README.md

+will be called.
+
+When children are passed, then the waypoint's size will be determined by the
+size of the contained children. The `onEnter` callback will be called when *any*


the waypoint's size will be determined by the size of the contained children

I assume that this isn't strictly true (e.g. if the children are absolutely positioned). Perhaps we should phrase this in a way that makes this clear, or provide a list of known cases where this statement is not true.

You're right. I'll add more details.

If we move forward with React.Children.only, this section of documentation will need to be changed a fair amount (i.e. simplified), so it might be worth waiting to see how that pans out before spending too much time here.

👍Ill wait to see how things pan out.

lencioni · 2017-01-29T16:03:06Z

README.md

+
+When children are passed, then the waypoint's size will be determined by the
+size of the contained children. The `onEnter` callback will be called when *any*
+part of the children is visible in the viewport. The `onLeave` callback will be


any part of the children is visible in the viewport

This is true, per axis I believe. In other words, if you have a page that scrolls in both directions and you have a waypoint with children that is vertically in the viewport but not horizontally, it will still fire.

Hm, I think I get what you mean, but your example is throwing me off. If something is in the viewport, then is it not by definition within the boundaries of both axes?

If something is in the viewport, then is it not by definition within the boundaries of both axes?

Yes, but (and I could be misunderstanding the implementation here) Waypoint is only checking viewport boundaries on a single axis (either vertical or horizontal). So, Waypoint's definition of "in the viewport" is really "in the viewport on a single axis and ignoring the other axis".

Ah, right. I see what you mean. What's here could be misleading, so I'll update this and attempt to capture this behavior.

lencioni · 2017-01-29T16:05:27Z

src/waypoint.jsx

+    return POSITIONS.inside;
+  }
+
+  // top is above the screen and bottom is below the screen


I agree, there is value in being consistent with terminology. It would be nice if this PR didn't make the terminology more cloudy at the least.

lencioni · 2017-01-29T16:07:43Z

src/waypoint.jsx

-      this._ref.getBoundingClientRect().top;
+    const { left, top, right, bottom } = this._ref.getBoundingClientRect();
+    const waypointTop = horizontal ? left : top;
+    const waypointBottom = horizontal ? right : bottom;


top/bottom is very vertically oriented terminology. I know you were just following the local pattern here, but what do you think about using waypointStart and waypointEnd instead?

I think that would require either a breaking change to the api (currently waypointTop is an exposed attribute) or inconsistent terminology between the internal codebase and the external api.

Ah yes, that is a good point.

lencioni · 2017-01-29T16:30:15Z

src/waypoint.jsx

@@ -333,13 +352,18 @@ export default class Waypoint extends React.Component {
   * @return {Object}
   */
  render() {
+    if (this.props.children) {
+      return <div ref={this.refElement}>{this.props.children}</div>;


I think that adding a div here will be potentially unexpected for consumers and problematic for styling purposes. For example, if I have some content that I want to be inline-block and I wrap it in a Waypoint, how would I achieve the styling I need?

I don't think we should allow people to pass in styles or classNames because of how much complexity that adds to people's codebases.

Instead, we could useReact.Children.only so the consumer is responsible for adding the wrapping element and styling it how they desire. To make this work, we will also need to add a refToMeasure prop, similar to what is outlined here: jsx-eslint/eslint-plugin-react#678 (comment)

What do folks think?

I think that sounds like a strong approach, personally. I would be willing to make that adjustment if desired.

I haven't used refToMeasure before, and I wasn't able to figure out what that would look like from reading those links. Could someone type up a very quick example showing what that would look like for this lib?

The primary benefit of using React.Children.only that I see is allowing users to customize any number of props of the wrapping node. With that said, I think it's likely that most developers will just want to apply styling to the wrapper, rather than any other props, so I'm not sure how much that benefit will matter to folks.

I understand, and have experienced, many of the problems described in the "Don’t pass CSS classes between components" post, yet I disagree with the conclusion that categorically denying use of class name / style props is the best solution. I don't want to get too much into that here (although it'd be a great conversation, I'm sure!), but I figured I'd mention that I'd be fine just going the more traditional route and using class name/style props.

Anyway, I'm likely also alright with the refToMeasure prop as long as it's not too complex nor confusing for the consumers of this lib.

Could someone type up a very quick example showing what that would look like for this lib?

Although not quite the same as what is outlined in the link I shared, in this case refToMeasure is just a generic prop that contains a ref to a DOM node to measure (there might be a better name). In practice it would probably look something like this:

<Waypoint refToMeasure={this.state.node} onEnter={this.handleWaypointEnter}> <div ref={node => { this.setState({ node }); }}> Something interesting would go here. </div> </Waypoint>

Yup. I'll go ahead and build that then?

I'm planning to also allow users to pass an element without a ref, and have one injected using cloneWithProps or equivalent, like so:

<Waypoint onEnter={this.handleWaypointEnter}> <div> Something interesting would go here. </div> </Waypoint>

in which div ends up with a ref created/managed by Waypoint.

An alternative might be to go with the div implementation by default, but allow the user to pass in a refToMeasure instead, in which case this.props.children could be rendered directly.

This is a bit more complex; I'm happy to write it if y'all are okay maintaining it 😉

I haven't followed all of the discussion around this, but I think the div alternative might be the right one at this point. It should solve most cases without too much headache. We should mention in the README that a div is added automatically, but that might be enough for this to become useful.

I think that's fine, but I think it would still be good to roll with React.Children.only because it will be more painful to make it more restrictive later than it will be to make it less restrictive if we decide later that we should have gone a different route. What do you think?

Let's just get this "right" from the get-go; <div> by default, with an "advanced escape hatch" of refToMeasure requiring React.Children.only and documentation reminding that a ref can't attach to a stateless component.

Makes usage easy for most people, and accommodates edge cases that are bound to occur. No reason to worry about adding/removing flexibility later.

Unfortunately, it turned out the refToMeasure approach doesn't work, because the ref of the child is null until after the parent has rendered.

On the other hand, cloning and injecting a ref does work after all. I was afraid of overwriting a ref, but it was easy to preserve.

lencioni · 2017-01-29T16:30:28Z

src/waypoint.jsx

    // We need an element that we can locate in the DOM to determine where it is
    // rendered relative to the top of its context.
    return <span ref={this.refElement} style={{ fontSize: 0 }} />;
  }
 }

 Waypoint.propTypes = {
+  children: PropTypes.oneOfType([PropTypes.element, PropTypes.array]),


Should this be PropTypes.node?

As the PR is currently yes (good catch!). If the React.Children.only proposal is adopted – and I think it should be – then it should be element, not node, so that a ref can be injected or detected.

catamphetamine · 2017-02-02T18:42:29Z

Looking forward to this new feature.
Good job collaborating on this you all.

rattrayalex · 2017-02-02T18:59:12Z

Thanks!

I'm planning to add the refToMeasure functionality discussed here: #152 (comment) . Probably tomorrow, maybe today.

I'll also incorporate a bunch of the nits, incl docs, outlined above. @jmeas would you prefer I add extra commits on top, or squash everything down into one?

rattrayalex · 2017-02-09T01:31:37Z

Alright, finally was able to implement a wrapper-free option. As I mentioned in an earlier comment, the original plan of refToMeasure didn't work because the child ref wasn't available on component mount (almost by definition).

Would love 👀 !

lencioni

Thanks for working on this!

lencioni · 2017-02-09T01:41:19Z

src/waypoint.jsx

    // We need an element that we can locate in the DOM to determine where it is
    // rendered relative to the top of its context.
    return <span ref={this.refElement} style={{ fontSize: 0 }} />;
  }
 }

 Waypoint.propTypes = {
+  children: PropTypes.oneOfType([PropTypes.node, PropTypes.array]),


This should be just PropTypes.node

I don't think so? The following is valid:

<Waypoint> <div /> <div /> </Waypoint>

It wraps the two child divs in a parent div.

PropTypes.node allows that. PropTypes.array here is redundant.

Anything that can be rendered: numbers, strings, elements or an array (or fragment) containing these types.

https://facebook.github.io/react/docs/typechecking-with-proptypes.html#react.proptypes

Ah, missed that. Thanks; amending now.

lencioni · 2017-02-09T01:41:56Z

src/waypoint.jsx

@@ -333,13 +352,30 @@ export default class Waypoint extends React.Component {
   * @return {Object}
   */
  render() {
+    if (this.props.noWrapper) {


This should only happen if also this.props.children

Yes, but someone who uses this.props.noWrapper without actually passing children should receive an error message – which will happen thanks to React.Children.only.

Ah, that's fair.

lencioni · 2017-02-09T01:42:47Z

src/waypoint.jsx

    // We need an element that we can locate in the DOM to determine where it is
    // rendered relative to the top of its context.
    return <span ref={this.refElement} style={{ fontSize: 0 }} />;
  }
 }

 Waypoint.propTypes = {
+  children: PropTypes.oneOfType([PropTypes.node, PropTypes.array]),
+  noWrapper: PropTypes.bool,


Why provide this option? Why not make this the default? I think that would simplify the API and be clearer.

Yeah, I struggled with that. Ultimately figured it would be confusing for too many users, especially beginners, since they would have to pass a single React Class-based Component or DOM Element.

The "INVALID" examples I included in the docs are all perfectly reasonable things to do, and allowing them makes ReactWaypoint more approachable.

Thoughts?

My opinion is that having multiple ways to do this will be more confusing, because it it less clear which one to choose and why you should choose one over the other. This also makes the documentation more complicated than it needs to be.

Since we have a good solution in the noWrapper branch, my preference would be to go with that only. I'd like to hear from others like @trotzig and @jmeas.

I'm in favor of not having the noWrapper prop. This is tricky though. On the one hand, I think we could go with the pragmatic choice of just adding a div wrapper, and solve 90+% of use-cases. But then at some point the div is going to become a problem, so maybe it's worth it to address upfront.

I think we should remove the prop and just go wrapper-less by default, unless the children being passed in is an array, a string, or a stateless component (function). Something like:

function getContent(children) { if (React.Children.count(children) > 1 || isStateless(children)) { return <div>{children}</div>; } return children; }

This is a simplified example, we still have to assign the ref. Also, I don't know how hard the isStateless function would be to implement.

I don't think it is a good idea to conditionally add a div if there are multiple children or of the children is a functional component because that would be a pretty surprising and unexpected thing for folks. I think it would be much better to provide a useful error message and always require an only child that is not a functional component. That is clear, consistent, and flexible.

That's unnecessary clutter of user's code.

I disagree about this. It makes it very clear what is going on and puts full control in the hands of the developer. Of course, consumers could choose to abstract this away in a component that wraps Waypoint and adds a div, or we could provide a separate component that does this for folks.

export default function WaypointDiv({ children, ...waypointProps }) { return <Waypoint {...waypointProps}><div>{children}</div></Waypoint>; }

sidebar: here's a related talk that I quite like http://2014.jsconf.eu/speakers/sebastian-markbage-minimal-api-surface-area-learning-patterns-instead-of-frameworks.html

I understand the pros of cons of each argument here. I like @trotzig 's conditional-wrapping solution, in that it tries to hide the complexity to the user. That can be useful for someone just getting started, as they have less to think about. @lencioni is probably right that at other times it could cause confusion when that same person doesn't understand why the div is added sometimes, but not others. I think the user still has full control here, because the situations where Waypoint wraps would always happen when it was absolutely necessary. It simply wouldn't work otherwise. A downside is that it adds some degree of complexity to Waypoint to manage that conditional, although I'm not sure how hard that would be to do.

@lencioni 's suggestion to never wrap is appealing because I like giving all of the control to the developer, even if the learning curve is slightly higher at first. I think a helpful error message could quickly help someone get up to speed with when it's necessary, and when it's not. Also, avoiding another prop is a good thing, too. But I also understand where @rattrayalex is coming from about requiring folks to put wrapping divs everywhere. That is a definite downside to this approach.

I think I'd be alright with either magically wrapping or not wrapping ever, but I'm not a fan of a noWrapper prop.

If I had to pick between the options, it'd be @lencioni 's suggestion to not add the wrapping prop, and require the user wrap it as needed. As for the WaypointDiv component, I'd probably prefer to not include it just to keep this library simpler, but I don't feel too strongly about it.

tl;dr: as long as we avoid noWrapper, I'm OK with anything else.

As for the WaypointDiv component, I'd probably prefer to not include it just to keep this library simpler, but I don't feel too strongly about it.

I agree. We could mention this technique in the documentation with a little code snippet if we wanted. That would likely be sufficient.

Alright; I guess I'm not the one who'll have to deal with any user frustration anyway ;-)

I'll try to put together some docs on the topic as well....

lencioni

I'm just marking this as changes requested at least until it is no longer marked as a WIP

Things are still being worked on

rattrayalex · 2017-02-09T21:10:03Z

Alright, I believe this to be ready for 👀 again

lencioni · 2017-02-09T21:31:31Z

README.md

 <Waypoint
  onPositionChange={this._handlePositionChange}
 />
 ```

+Waypoints can take a child, allowing you to track when a section of content


It might be a good idea to be explicit here that it must be an only child.

Hmm, do you think a child is inadequately clear?

I considered phrasing like "a single child" but was concerned that might sound like the following was illegal:

<Waypoint> <div> <GrandChild /> <AnotherGrandChild /> </div> </Waypoint>

It is also explicit in the Children section

Yeah what you have is probably fine here. If there's confusion, we can adjust the docs then.

lencioni · 2017-02-09T21:31:45Z

README.md

@@ -248,6 +261,31 @@ the `onLeave` and `onEnter` callback will be called. By using the arguments
 passed to the callbacks, you can determine whether the waypoint has crossed the
 top boundary or the bottom boundary.

+## Children
+
+If you don't pass children into your Waypoint, then you can think of the


s/children/a child/

lencioni · 2017-02-09T21:32:22Z

README.md

+(Note that this is measured only on a single axis; strangely positioned elements
+may not work as expected).
+
+Deciding whether to pass children or not will depend on your use case. One


Should maybe be "a child" here and lower as well?

lencioni · 2017-02-09T21:32:59Z

README.md

+may not work as expected).
+
+Deciding whether to pass children or not will depend on your use case. One
+example of when passing children is useful is for a scrollspy. Imagine if you


Should "scrollspy" link somewhere for folks who are unfamiliar with the term?

I'm not finding a concise explanation with a quick google, and the next sentence does a decent job of explaining the use-case IMO

One idea would just be to link to the Boostrap docs. It might seem weird, but:

I hadn't really heard the term before them

they do a good job at explaining it

so many people use, or have used, Bootstrap

They version their docs, so we could "lock it down" to a specific version to avoid issues if they remove the scrollspy ever.

...maybe this is a bad idea.

I kinda like that. I'll add it

lencioni · 2017-02-09T21:34:49Z

src/waypoint.jsx

+ * @param {React.element} Component
+ * @return {bool} Whether the component is a DOM Element
+ */
+function isDomElement(Component) {


nit: I'd prefer for the DOM acronym to be fully capitalized here.

lencioni · 2017-02-09T21:36:10Z

src/waypoint.jsx

+ * @return {bool} Whether the component is a DOM Element
+ */
+function isDomElement(Component) {
+  return (typeof Component.type === 'string');


Is this a reliable check? Is there some documentation we could link to?

"When an element's type is a string, it represents a DOM node with that tag name"

from the docs

I'll add to the jsdoc

lencioni · 2017-02-09T21:37:37Z

src/waypoint.jsx

@@ -333,13 +359,30 @@ export default class Waypoint extends React.Component {
   * @return {Object}
   */
  render() {
+    if (this.props.children) {
+      const child = React.Children.only(this.props.children);
+      if (!isDomElement(child)) {


I wonder if we should move this check into componentWillMount and componentWillReceiveProps. Not sure if throwing an error in render will cause weird issues with React. Also, using CWRP will allow us to only do this check when children has changed.

Sure, sounds good

lencioni · 2017-02-09T21:38:35Z

src/waypoint.jsx

+      const ref = (node) => {
+        this.refElement(node);
+        if (this.props.children.ref) {
+          this.props.children.ref(node);


nit: It might be nice to destructure children at the top of render.

rattrayalex · 2017-02-09T22:17:28Z

@lencioni I believe all comments have been addressed; take another look?

jamesplease · 2017-02-09T22:20:56Z

README.md

+[boundary](#offsets-and-boundaries), then the `onEnter` or `onLeave` callbacks
+will be called.
+
+If you do pass a child, it must be a single DOM Element (eg; a `<div>`).


If Components are not valid as the child (they're not, right?), then it might be a good idea to explicitly say that here. i.e.;

If you do pass a child, it must be a single DOM Element (eg; a <div>), and not a Component (eg; <MyCustomComponent>).

Nice, clarified (it had said this elsewhere, but I consolidated)

jamesplease · 2017-02-09T22:24:43Z

README.md

+The `onEnter` callback will be called when *any* part of the child is visible
+in the viewport. The `onLeave` callback will be called when *all* of the child
+has exited the viewport (eg; both the top and bottom are either above or below).
+(Note that this is measured only on a single axis; strangely positioned elements


"strangely positioned" is pretty vague. It's tough to word, but maybe something like:

Note that this is measured only on a single axis. What this means is that for a Waypoint within a vertically scrolling parent, it could be off of the screen horizontally yet still fire an onEnter event, because it is within the vertical boundaries.

or something to this effect.

jamesplease · 2017-02-09T22:27:17Z

I just noticed how long this PR has been going on for:

I'm really thankful that you've kept up with it @rattrayalex . I know it can be tiring when a PR drags on forever. This is really great stuff, though, and we're so close to landing it now!

rattrayalex · 2017-02-09T22:35:13Z

Hah, we broke 100! Thanks, @jmeas , I appreciate that 😄 I think I've reimplemented the feature at least 3 or 4 different ways by now; so it goes. Happy with where it ended up!

jamesplease · 2017-02-09T22:37:05Z

README.md

+in the viewport. The `onLeave` callback will be called when *all* of the child
+has exited the viewport.
+
+(Note that this is measured only on a single axis.


smallest teeniest ittiest nit ever...pretty early line break here :)

jamesplease · 2017-02-09T22:37:40Z

README.md

+(Note that this is measured only on a single axis.
+What this means is that for a Waypoint within a vertically scrolling parent,
+it could be off of the screen horizontally yet still fire an onEnter event,
+because it is within the vertical boundaries.).


.).

seems like weird punctuation

lencioni · 2017-02-09T23:30:41Z

src/waypoint.jsx

@@ -178,6 +207,17 @@ export default class Waypoint extends React.Component {
    clearTimeout(this.initialTimeout);
  }

+  _ensureChildrenIsSingleDOMElement(children) {


Since this function doesn't reference this, it doesn't need to be on the class. Move it up to the module.

lencioni · 2017-02-09T23:31:54Z

One small change and then I think this is ready to go. I'd like to hear from @trotzig on where we landed before merging though.

lencioni · 2017-02-10T00:43:42Z

src/waypoint.jsx

@@ -158,6 +202,10 @@ export default class Waypoint extends React.Component {
    }, 0);
  }

+  componentWillReceiveProps(nextProps) {
+    ensureChildrenIsSingleDOMElement(nextProps.children);


I wonder if it would be worth only calling this if this.props.children !== nextProps.children

I don't think there would be a downside to that...

trotzig

Thanks everyone for all your input on this PR! I'm happy we ended up where we did, knowing the long list of edge-cases that could come up.

trotzig · 2017-02-13T21:34:37Z

README.md

+
+The `onEnter` callback will be called when *any* part of the child is visible
+in the viewport. The `onLeave` callback will be called when *all* of the child
+has exited the viewport.


I like this section.

trotzig · 2017-02-13T21:35:24Z

README.md

+and instead put the waypoint above or below the content, then you will not
+receive an `onEnter` callback (nor any other callback from this library).
+Instead, passing this long content as a child of the Waypoint would fire the `onEnter`
+callback when the page loads.


This is a good piece of context.

jamesplease · 2017-02-13T21:42:18Z

Woo!! 🎉 👯 💃 🍰 🎂

rattrayalex · 2017-02-13T22:15:56Z

Woohoo! Thanks so much everyone!

rattrayalex mentioned this pull request Jan 25, 2017

Allow children to be wrapped inside waypoint. This makes it easier to… #41

Closed

rattrayalex force-pushed the accept-children branch from 6593826 to 1396f56 Compare January 25, 2017 20:00

trotzig reviewed Jan 26, 2017

View reviewed changes

rattrayalex force-pushed the accept-children branch from 1396f56 to 791dec9 Compare January 26, 2017 18:33

jamesplease reviewed Jan 26, 2017

View reviewed changes

jamesplease previously approved these changes Jan 26, 2017

View reviewed changes

Accept children and emit waypointBottom (w/ @trotzig)

ec87515

rattrayalex force-pushed the accept-children branch from 0e469d5 to ec87515 Compare January 26, 2017 21:34

jamesplease force-pushed the accept-children branch from bd3f248 to d97a6c0 Compare January 29, 2017 05:15

Update documentation for accepting children

b0d7d3e

jamesplease force-pushed the accept-children branch from d97a6c0 to b0d7d3e Compare January 29, 2017 05:17

lencioni reviewed Jan 29, 2017

View reviewed changes

jamesplease mentioned this pull request Feb 6, 2017

onLeave isn't fired when onPositionChange does #155

Closed

lencioni reviewed Feb 9, 2017

View reviewed changes

lencioni requested changes Feb 9, 2017

View reviewed changes

rattrayalex force-pushed the accept-children branch 2 times, most recently from bb5bea6 to c05fee1 Compare February 9, 2017 21:14

lencioni approved these changes Feb 9, 2017

View reviewed changes

rattrayalex force-pushed the accept-children branch 2 times, most recently from c32770b to ab6a411 Compare February 9, 2017 22:17

jamesplease reviewed Feb 9, 2017

View reviewed changes

rattrayalex force-pushed the accept-children branch from ab6a411 to 214e815 Compare February 9, 2017 22:26

rattrayalex force-pushed the accept-children branch from 214e815 to a65cfc4 Compare February 9, 2017 22:28

jamesplease reviewed Feb 9, 2017

View reviewed changes

rattrayalex force-pushed the accept-children branch from a65cfc4 to 8ffe399 Compare February 9, 2017 22:38

lencioni approved these changes Feb 9, 2017

View reviewed changes

Children accepted must be a single a DOM Element

52e14c6

rattrayalex force-pushed the accept-children branch from 8ffe399 to 52e14c6 Compare February 9, 2017 23:40

lencioni reviewed Feb 10, 2017

View reviewed changes

trotzig approved these changes Feb 13, 2017

View reviewed changes

trotzig merged commit c7a6654 into civiccc:master Feb 13, 2017

jamesplease mentioned this pull request Feb 13, 2017

A way to create scrollspy #112

Closed

rattrayalex deleted the accept-children branch February 13, 2017 22:15

rattrayalex mentioned this pull request Mar 27, 2017

Provide way to use single quotes in jsx prettier/prettier#1080

Closed

jamesplease mentioned this pull request Mar 28, 2017

support for styled-components #168

Closed

Accept children #152

Accept children #152

Conversation

rattrayalex commented Jan 25, 2017

rattrayalex commented Jan 25, 2017

trotzig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex Jan 26, 2017 • edited Loading

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

trotzig commented Jan 26, 2017

rattrayalex commented Jan 26, 2017

rattrayalex commented Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

rattrayalex Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease commented Jan 26, 2017 • edited Loading

rattrayalex commented Jan 26, 2017

jamesplease commented Jan 26, 2017

rattrayalex commented Jan 26, 2017

jamesplease commented Jan 28, 2017

jamesplease commented Jan 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesplease Jan 29, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

catamphetamine commented Feb 2, 2017

rattrayalex commented Feb 2, 2017

rattrayalex commented Feb 9, 2017

lencioni left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rattrayalex Jan 26, 2017 •

edited

Loading

rattrayalex commented Jan 26, 2017 •

edited

Loading

rattrayalex Jan 26, 2017 •

edited

Loading

jamesplease Jan 26, 2017 •

edited

Loading

jamesplease commented Jan 26, 2017 •

edited

Loading

jamesplease Jan 29, 2017 •

edited

Loading

rattrayalex Jan 29, 2017 •

edited

Loading

jamesplease Feb 9, 2017 •

edited

Loading