Feature/bounds #109
Feature/bounds #109
Conversation
* Use ref instead of findDOMNode * Better compatability with React 15.2.x
thomassuckow
force-pushed
the
feature/bounds
branch
from
5097201 to
2d58e02
Compare
| let contextHeight; | ||
| let contextScrollTop; | ||
| if (this.scrollableAncestor === window) { | ||
| contextHeight = window.innerHeight; | ||
| contextScrollTop = 0; | ||
| } else { | ||
| contextHeight = this.scrollableAncestor.offsetHeight; | ||
| contextScrollTop = ReactDOM | ||
| .findDOMNode(this.scrollableAncestor) | ||
| contextScrollTop = this.scrollableAncestor |
There was a problem hiding this comment.
Worth noting that this would be a breaking change
There was a problem hiding this comment.
I don't understand how this changes anything.
this.scrollableAncestor is a dom element so
ReactDOM.findDOMNode(this.scrollableAncestor) === this.scrollableAncestor
There was a problem hiding this comment.
Ah, you're right. I was thinking that passing a component instance in as a scrollableAncestor would work (by leveraging findDOMNode), but I took another look at the source and saw that it would error earlier on in the code.
Ignore me! 🙈
What's this issue? I do think it's a good idea to move away from findDOMNode, as React might deprecate it. As an aside, I find it's usually easier to land PRs if they're separate things tho (since a single problem can hold up merging everything else 😄 ) |
|
https://github.com/apollostack/meteor-starter-kit/issues/16 describes the issue I was seeing while working on these changes. For some reason building react-waypoint on my machine caused ReactDOM.findDOMNode to throw an invariant exception. |
|
I would normally separate them, but the changes overlap so they would have to be submitted one at a time. I did pull the third commit (No forced layout on mount/update) though because it isn't strictly necessary and breaks the tests. |
| }); | ||
| this.props.onLeave.call(this, { | ||
| currentPosition, | ||
| previousPosition: POSITIONS.inside, | ||
| event, | ||
| waypointTop: bounds.waypointTop, | ||
| viewportTop: bounds.viewportTop, | ||
| viewportBottom: bounds.viewportBottom, |
There was a problem hiding this comment.
I think you could use an object spread here (and in two places above) to avoid having to repeat the bounds properties:
this.props.onLeave.call(this, {
currentPosition,
previousPosition: POSITIONS.inside,
event,
...bounds,
});
There was a problem hiding this comment.
I could, but I would need to add a few babel plugins. If you would prefer that syntax I can do so.
https://babeljs.io/docs/plugins/transform-object-rest-spread/
|
Thanks @thomassuckow, this all looks good to me. Good call on pulling the third commit (makes this PR easier to merge), although I'd love to see a separate PR for that! I'll let you decide if you want to take action on my comment to DRY up src/waypoint.jsx a little before merging. |
|
DRY:
Unless I am mistaken the above is the only DRYing change, if I am wrong point it out. It is also dependant on whether this project wants to add more babel plugins or if there is some reason not to. |
|
Ah, I thought we had that wired up already. Let's merge this without it. We can change that later on. |
|
rest/spread is only a stage 2 proposal right now. I think we should wait until it hits stage 3 to bring it in. https://github.com/sebmarkbage/ecmascript-rest-spread |
|
I included this change in v3.1.0 (just released). Thanks for the contribution @thomassuckow! |
Resolve adding bounds values as described in #107
Also resolves an issue with findDOMNode "Uncaught Invariant Violation: getNodeFromInstance"