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

[khankuan]

… use waypoint without worrying about the alignment of the dummy span.

[kevinkehl]

Thanks for submitting this pull request. Can you give a little more information to help me understand your use case for this change?

[khankuan]

Hey, thanks for reading ;) I had an issue where the way point was aligning to the bottom of the row. It's kinda odd to have to align the way point. So my code ended up working well with

render() {
    return (
        <Waypoint... >
            <p>my paragraph <p>
        </Waypoint >
    );
} 

It's also convenient if I do not need to wrap the render with a div. Furthermore, I'm not sure if Waypoint works for an area (rect). If so, it's probably useful to be wrapping an element.

Cheers :)

[trotzig]

I understand that styling might be an issue with the waypoint (we've seen this come up before in #34), but I'm not convinced this is the right solution. For once, the waypoint needs to be of 0 height for it to be easy to reason about when it enters and leaves the screen. What if the waypoint element is 50 pixels tall -- do we call onEnter when the bottom edge enters? Or when the entire element has entered?

[khankuan]

I'm weighing the pros and cons of allowing a children to be wrapped:

Pros:

• Allowing a waypoint to take the shape of the actual element that the user is concern with
• Remove assumption for span to be used as the waypoint element. A user could potentially have css on a span.
• More reactish pattern where components naturally are nested and thus could have children.
• Reduce the need to have a wrapper component, in the case where I have a Waypoint + another element, now i could wrap the other component into Waypoint without creating a wrapper parent.

Cons:

• Ambiguity when onEnter on the bottom edge.

I'm thinking in the case of ambiguity, i would suggest we can change the computation to include the component height. That makes it easier to support a Waypoint that is more '2d'.

[khankuan]

Just a comment, allowing children has allowed flexibility to solve issues in several occasions in production without needed to change css or additional unnecessary lines of code. So i'm not sure why it isn't something handy for this lib.

[janpaul123]

Throughout the component, we make the assumption that Waypoint has no height. So this would require a bunch of fixes throughout the component. And we'd probably need a prop to switch between firing onEnter on partially entering and fully entering. After doing that I'd be OK with this change. I agree it does seem useful (especially since it's not super easy to get this behaviour by combining two Waypoints around a component or so).

[khankuan]

Ohh, I actually meant that it can contain a child but not necessarily consider the bottom of the child at all and remains the same as before. Hence, only an additional line is sufficient.

[Sicria]

This is the behavior that I expected of this package.

One use-case that I'm dealing with currently is when I want to have a content block surrounded by waypoints, if the user reloads the page, it does not capture that waypoint and thus does not trigger any actions until a user hits one of those single spans.

I think that this change would solve that issue, as when the user reloads the page, the content block would be wrapped with the waypoint it would be triggered.

[trotzig]

I see a few use-cases mentioned here that we think will benefit from passing in children, and I just want to make sure that we're on the same page. @Sicria mentions reloading the page with a content block benefitting from being able to pass in children. Can you elaborate a little bit, perhaps with an example? I don't fully see the connection to a children prop, and I don't actually understand the problem you are running into (it might be a bug in waypoint?).

@khankuan makes a few points about better control over how the waypoint is rendered (the span sometimes being a problem). I've seen this come up a few times, and I'm open to a PR fixing that, complete with tests and documentation.

The children prop seems to be a side-effect from trying to solve other issues. But as @janpaul123 mentioned, we'd be open to a PR for this as well (possibly merged with the PR for added control over the waypoint rendering). We need to do this right however. If we allow children to be passed in, we need to take the height of the container into account, and drop the assumption that the waypoint is 0px tall. I'm a fan of composition however, and it's fairly simple to create a wrapper component around two waypoints to achieve the same thing:

<div>
  <Waypoint onEnter={() => this.setState({ topVisible: true }} />
  Some content
  <Waypoint onEnter={() => this.setState({ bottomVisible: true }} />
</div>

I'm sorry if we're coming across as too protective (I can definitely understand if you feel that way). I want to thank @khankuan for being persistent and responsive to our continued questioning, and for making some really good points. Where most people would just leave, you kept moving the conversation forward. Let's keep ironing this out, I'm sure we can land at something good! 😄

[Sicria]

The problem that I've come across is that if the content block is large (say 100px) then when you reload the page you don't trigger any waypoints. I've currently wrapped my content blocks with waypoints but I still run into the reload issue.

[trotzig]

Did you mean 1000px? 100px doesn't sound "large" to me... :)

Also, at what stage in reloading a page do you expect the waypoints to be triggered? In the scenario you outline (assuming 1000px content), I would expect the top waypoint to trigger on load/mount, and the lower one not to trigger until you scroll down.

Are you doing any lazy loading on the page? Do images have a fixed height or do they start out 0px and grow when they've been downloaded? Those are just some general things that can cause unexpected triggering/non-triggering of waypoints.

[Sicria]

I mean 1000px (sorry, typo).

I've got a longform sales page with different sections. Think (About, Features, Pricing etc..) When a user is in that section the header navigation needs to change using the waypoints. It works fine when scrolling up and down the page, but if you reload in the middle of one of the sections it does not trigger the enter which would be ideal, because the user entered the section when they reloaded the page.

No lazy loading done currently.

[trotzig]

Closing due to inactivity.

[mikeengland]

I'd also like to see this library allow children to be rendered inside a waypoint. Currently, if waypoints are used above for example a 200px part of a form - when a user scrolls slightly above this waypoint, the previous waypoint does not trigger. Extra management code is required to handle which anchor is currently active in this case.

[trotzig]

I reopened this without realizing it was a PR.

@mikeengland: We are open to a PR adding support for children. I'll direct you to #112 for a bit of context if you decide to accept the task.

[rattrayalex]

I'm interested in tackling this.

[jamesplease]

Go for it, @rattrayalex ! Feel free to ping me if you run into any issues. I'd be happy to help if i can.

[rattrayalex]

Thanks @jmeas ! WIP PR is here: #152