[improvement] Move Waypoint to a higher order component approach

[bySabi]

Hello.

I fork Waypoint and reimplemented it like a higher order component. Now I cant use this way, Ex.

const Comp = (props) => (
  <div {...props}>
     <Child1/>
     <Child2/>
     <Child3/>
 </div>
);

const Scroller = waypoint(Comp);

const ScrollerPage  = () => (
   <div>
      <Scroller 
        onEnter={() => console.log('enter Comp')}
        onLeave={() => console.log('leave Comp')}
      />
   </div>
);

I add only:

      const nodeHeight = this._DOMNode.getBoundingClientRect().height;
      .
      .
      if (contextScrollTop <= waypointTop + nodeHeight + thresholdPx &&
        waypointTop - thresholdPx <= contextBottom) {
        return Waypoint.inside;
      }

to _currentPosition()

Is working good on a internal project.

Please tell me if looks good and will make a PR.

Fork repo on this link: https://github.com/bySabi/react-waypoint

Thanks for your hard works on this.

[trotzig]

Interesting approach @bySabi. Just to see if I'm understanding correctly, would this be equivalent to placing the waypoint after all children in the Comp example?

const Comp = (props) => (
  <div {...props}>
     <Child1/>
     <Child2/>
     <Child3/>
    <Waypoint ... />
 </div>
);

While I'm personally a fan of HOC, I'm not sure it's the right tool for the job here. Personally, I think that allowing a waypoint to take children is a more straight-forward approach to solving what you are trying to do with the HOC.

const Comp = (props) => (
  <Waypoint ...>
     <Child1/>
     <Child2/>
     <Child3/>
 </Waypoint>
);

This is discussed in length over at https://github.com/brigade/react-waypoint/pull/41. Basically, I'm open to such an addition, but only if we change callback triggering to take the height into account. I.e. onEnter should only fire once the entire element is visible, and onLeave should only fire once the entire component has left the viewport. Taking an arbitrary tall element into account opens up for some tricky edge-cases that we've been avoiding so far. For instance, what happens if the wrapped content is taller than the screen? Should the onEnter callback get fired or not?

[bySabi]

In this case HOC and children aproach bring a similar solution for same "issue?". I agree on all the points you mentioned. At what point fire Enter/Leave on wrapped components don´t have a clear answer. Make waypoint 0px is a salomonic decision. But put two Waypoint listeners at component edge could have performance penalty, don´t you think?

[trotzig]

Performance might be an issue, but it's just speculation. We would have to profile the code in order to answer that question. At brigade.com, we have 20+ waypoints on some pages, and there's little to no experienced lag there.

Can you explain what you mean by salomonic? (It's a word I don't understand, and English isn't my first language).

[bySabi]

Sorry! is a typo, is solomonic from Solomon (https://en.wikipedia.org/wiki/Judgment_of_Solomon) :-)

Right!, modern browser probably handle many event listeners without hastle. Even so IMO waypoint must have a more room for developer experimentation. I like HOC cause Waypoint component remain the same and now have a functional scroll spy behaviour to attach to any component. We only need figure out the correct API for express at what point on the ancestor-component relation we must fire callbacks. Any suggestion about?

Maybe fire callbacks must be parameterized relate to Wrapped Component center line. I working on this right now :-)

[trotzig]

Most HOC that I've seen pass in some prop to the child they're wrapping. In your example the child isn't benefitting from being wrapped, that's why I think a regular children approach is more straightforward.

I think the power of Waypoint in its current form is that it's simple enough while still allowing it to be flexible enough to support a multitude of use-cases.

As for when callbacks need to be fired when wrapping children is up to you to figure out based on your use-case. The center line is an interesting idea, and maybe that's what we should aim for!

[bySabi]

Hi @trotzig actually this is my only one HOC that no pass nothig. Probably I thought, if children are not 0px anymore wayPOINT semantically not fit at all.

I make a new PR to my fork on function _currentPosition.

This change take into account the 'children' height and determine where is it above, inside or below of "visible area" comparing agains children height middle point. If waypoint children height is less than half of "visible area" all full children must be inside this area to return Waypoint.inside If waypoint children height is greater than half of "visible are" children will be 'inside' when her top point (waypointTop) reach the center point of "visible are". This cover the case when children size it is bigger than container.

This need a future refactor but it is working.

Just want to know is _currentPosition changes are ok for waypoint team before make a PR.

[trotzig]

Just to better understand why you need this, can you explain your use-case a little?

[bySabi]

I have multiple components of this type:

import React from "react";
import classNames from "classnames";
import { mediaQuery } from "react-media-query";
import { mdUp } from "react-media-query/lib/bootstrap4-media-query";
import { waypoint } from "react-waypoint";

...
const Code = waypoint(props => (
  <Section {...props}>
    <Code lang="jsx" block>{sampleCode}</Code>
  </Section>
));
...
const SampleCode = mediaQuery(class SampleCode extends React.Component {
   onEnter = () => {/* onEnter code */};
   onLeave = () => {/* onLeave code */};

   render() {
     const mediumUp = this.props.md.mdUp ? 'hidden' : 'show';
     const _className = classNames(this.props.className, mediumUp);

     return <Code onEnter={this.onEnter} onLeave={this.onLeave} className={_className} /> 
   }
}, { mdUp });

...
const App = props => {
  ...

  <SampleCode className="main app sample1" />

  ...

  <SampleCode className="main app sample2" />

  ...

  <SampleCode className="main app sample3" />

  ...
}

If I were to build this module with current Waypoint component it will look like this.

const waypoint = C => {
  return function _waypoint(props) {
    const { onEnter, onLeave, ..._props } = props;
    return (
      <div>
        <Waypoint onEnter={onEnter}/>
        <C {..._props}/>
        <Waypoint onLeave={onLeave}/>
      </div>
    );
  }
}

...
const Code = waypoint(props => (
  <Section {...props}>
    <Code lang="jsx" block>{sampleCode}</Code>
  </Section>
);
...

With current Waypoint component, waypoint hoc:

• Will need 2 additional tags "span" and one container "div"
• Each Waypoint need custom threashold´s for callback fire when wrapped component enter on viewport zone.

I agree that Waypoint is powerful on his simplicity and cover a majority of use cases but sometimes many developers feel a little more is needed, #41, #87

Proposed HOC don´t touch current Waypoint behaviour, just make it more composable and component aware.

Before this I was looking for a scrollSpy solution and even wrote my self that create a only one event listener with function throttle, but eventually I move to Waypoint due to his simple, clean and tested code.

I take quite some time looking at Waypoint code just to make sure if this really add some improvement.

[trotzig]

Thanks for providing that example. I'm a little confused though, because the HOC you've implemented doesn't seem to support your use-case where you have two waypoints (one above and one below).

I've seen the issue of allowing children come up a few times, and I think the most useful addition we could make to waypoint at this time is to allow children to be passed in, and take the height of the element into account when calculating its position. That seems like it would help the use-case your explaining as well (with the two waypoints surrounding an element). The centering-approach you've taken in the HOC is interesting as well but I don't think that behavior is what most people would expect from a children-aware waypoint. And I still don't see a strong use-case for it.

[bySabi]

children approach is fine too :-) How you will handle the case when element height is more bigger than scrollable ancestor?

[trotzig]

Yeah, I don't know, it's tricky. I don't have a clear use-case in mind, but here's what I think we could do:

• Trigger onEnter when the bottom edge is visible (when scrolling down) or the top edge is visible (when scrolling up).
• Trigger onLeave when the bottom edge leaves the screen (when scrolling up) or the top edge leaves the screen (when scrolling down)

With this approach, we don't have to worry about how tall the waypoint element is. Though I don't know if it satisfies your needs. We could also consider this approach:

• Trigger onEnter when the both edges (top, bottom) are on screen
• Trigger onLeave when the both edges are off-screen

With this approach, there's a chance that the waypoint never fires onEnter, despite being clearly visible on the screen (because it's too tall).

A third approach is the one you explored with your HOC: making the waypoint trigger on the center point of the element. This approach doesn't suffer from the height issue either.

Again, you're in best position to decide what approach will fit your use-case best. We can perhaps ask @khankuan and @Sicria for input, since they've shown an interest in the children-approach too.

[bySabi]

Hi @trotzig

Actually my implementation don´t trigger on center point of element, it just use element middle point for recalculate context scroll area. Is like second approach you describe, onEnter only will fire if both edges are on screen. But if element height is more bigger than half of the context height:

• onEnter will fire when element top reach context center point
• onLeave will fire when element bottom leave context center point.

Behaviour on big elements is a litte 'cosmetic', visually speaking.

My use-case is, probably, avoid where ever I can adjust fire callbacks with threshold hardcoded values.

I´m with you," is really tricky!"

Lets see what others developers think.

Thanks for take time to anwser here.

[khankuan]

Hope i can contribute some thoughts :) I like the idea of thinking about the problem as having 2 waypoints. Thus, the permutations of events are essentially { enter/leave, above/below, top-edge/bottom-edge }. The library current has onEnter and onLeave callbacks.

1) We could add more params to the callbacks, perhaps something like topEdge=true, bottomEdge=true and for Waypoints that does not have height, we can set both values to true.

2) We can add new props onTopEnter, onBottomEnter, onTopLeave, onBottomLeave. These are triggered together with onEnter and onLeave and it is the choice of user to decide to go with something broader like onEnter or something more specific like onTopEnter.

Personally i feel that 2) is nicer so that the jsx itself is more declarative and the reader do not have to dig into the handler method to figure out both cases.

In reference to the children method, i'm assuming that topEdge refers to the top of first child while bottomEdge refers to the bottom of the last child.

[bySabi]

@trotzig @khankuan I revert _currentPosition function to original. Not more tricky behaviour based on element height size, is 0px again. But I think is useful know some DOM info like waypointTop and contextScroll top and height. This way I can customize components behaviour based on this DOM values, Ex:

• trigger animations when waypointTop is on the middle of viewport .

Is this changes fit on waypoint 'way' I will make a PR, please review it.

https://github.com/bySabi/react-waypoint/blob/master/src/waypoint-hoc.jsx#L178

Probably function name and vars need a litte refactoring

[bySabi]

commited another change to my fork. HOC pass prop _scrolled to wrapped components. This become react-waypoint a real scrollspy. Maybe this time HOC approach is justified :-)

https://github.com/bySabi/react-waypoint/blob/master/src/waypoint.jsx

[trotzig]

I'm not sure I'm following along now... Can you point me to a few commits so that it's easier to see the changes you are introducing?

[bySabi]

OK.

This commit pass more DOM data to callbacks on callbackArg https://github.com/bySabi/react-waypoint/commit/066184aab5a2686d2cda8b2d4da8c6d58b838b53#diff-159cb00bc1cbdf438aa0985561f6d9c1L85

next commit pass scroll state prop to childs, this would justified HOC approach: https://github.com/bySabi/react-waypoint/commit/45039de170e3c983fc044c8fac43ddd0b5e31e12#diff-159cb00bc1cbdf438aa0985561f6d9c1L33

Current master commit add some code to prevent infinite loops and render updates on Waypoint component.

[bySabi]

Latest commit https://github.com/bySabi/react-waypoint/commit/142c63baa9abbeed63c4cd6905aa185e3e27541e seens works fine. But need some 'specs' changes to reflex new API. Example of test fails ...

Expected spy onEnter to have been called with [ Object({ currentPosition: 'inside', previousPosition: null, event: null }) ] but actual calls were [ Object({ waypointTop: 98, contextHeight: 100, contextScrollTop: 8, currentPosition: 'inside', previousPosition: null, event: null }) ].
        at Object.<anonymous> (/Users/flx/Dropbox/WIP/react-waypoint/tests.webpack.js:155:36)

[bySabi]

Latest commit bySabi@50edd45#diff-159cb00bc1cbdf438aa0985561f6d9c1R1 add throttle handleScroll options. Recommended for performace tweaks.

[jamesplease]

@bySabi rather than referencing commits here, it might be easier for folks to follow along if you open a PR, and we track the progress there.

I'm :+1: to the idea of allowing a Waypoint to have children, and taking the height of the element into consideration. If there's anything I can do to help out with that effort, let me know.

[bySabi]

@jmeas my fork is almost ready but I can´t make a PR cause 'test' are not finnished, not time and Jasmine knowledge. Maybe you can helpe me in that apart.

I don´t think take height into account is a good idea after all. Is not a good idea on Waypoint code but is a must on some use-case, that´s why my current fork encourage waypoint higher-order component. This way ensure waypoint remain 'generic' but some use-case build on top of it.

I have been working on a scrollspy HOC build on top of waypoint HOC for easy build use-cases: sticky header, sticky footer, animate on reach viewport center, any behaviour you could imagine. Is not published yet but I can if someone request it.

I found a use-case that I think is tricky to solved with current Waypoint without a bit of hacky: A Waypoint below a component with a big background image that push sibling components out of viewport. In that case you must scroll to reach Waypoint onEnter point, this´s expected behaviour but on initial render sibling top component background is not take into account then Waypoint sit on viewport and onEnter get fired. Already mentioned scrollspy HOC eat this undesired case transparently to wrapped component.

If anyone have any question about this proposal and implementation don´t hesitate to ask.

[bySabi]

Created PR: https://github.com/brigade/react-waypoint/pull/92

[bySabi]

@trotzig what you think about my current fork: https://github.com/bySabi/react-waypoint-hoc ? I´m planning a PR with it but I not sure is react-waypoint team want follow the path of the generic scroll handler and make Waypoint a use-case of it. If the team decide follow this path would be with HOC or children approach??

[bySabi]

created v3 branch PR https://github.com/brigade/react-waypoint/pull/105