Improvements to horizontal viewport check #15
Conversation
|
Hi Mudit -- apologies for the last pull request. Made a mistake. Here's an updated request. Thank you for this plugin! It works great overall. I had a small issue when checking whether an element is horizontally within a viewport in a very specific case. I have an absolutely positioned, horizontally scrolling The plugin worked perfectly when checking the viewport's right bound, but didn't work after scrolling the inner content and checking the viewport's left bound. It always showed all elements that had been scrolled out of the viewport as "in" the viewport. I believe the issue was the check that I adjusted in this pull request. It was checking the absolute value of the element's left bound against the viewport. I updated it to check the element's left against the viewport's right bound, and vice versa. If there is some reason this doesn't work, please let me know (and feel free to reject the pull request). I ran the tests and they all seem to run properly, but I don't think there's a test for the specific case I mentioned. Writing a test for that case may be complicated, as it requires a lot more elements and adjusting the scroll position. Thanks! |
|
@npafundi could I urge you to add a test for your use case? Sorry about the delay. |
|
@zeusdeux - It may take me some time, but I'll do my best to get around to it. It may require a separate test file, and I'll have to look into how isInViewport handles tests. |
|
@npafundi sounds good 👍 |
|
@npafundi could you check if your use case is resolved? Pushed a few fixes today and bumped the version to |
npafundi
force-pushed
the
master
branch
3 times, most recently
from
a0c2d48 to
30c2258
Compare
|
Hey @zeusdeux, sorry for the very long delay. The recent updates haven't resolved the problem, but I've updated this pull request with a new test suite. The new tests will fail when run against the current version of isInViewport, but will succeed with the changes to the calculations in this fork. The issue occurs when a viewport contains horizontally-scrollable content. I've made a CodePen in which you can see the desired behavior: [http://codepen.io/npafundi/pen/wBNrop] and another with current behavior: [http://codepen.io/npafundi/pen/XJOzJd]. As you scroll the list, isInViewport should only return a list of items which are currently visible. Currently, as you scroll the list, isInViewport returns items which have since scrolled beyond the bounds of the viewport. This fork fixes that issue, so only the content currently in the viewport is returned. If you'd like me to make any changes, let me know. I'm not sure why the Travis build is failing -- all the tests complete successfully for me when run locally. Any idea what may be causing this? I'm happy to fix it if you can point me in the right direction. Thanks again for building isInViewport! -Nick |
|
@npafundi On merge with current Also, could you squash your commits into one and write up a proper commit message like the one you have in 381242d? Other than that it LGTM! 👍 PS: This is what I see in the failing tests: + expected - actual
+"7 8 9 10"
-"7 8 9 10 passes: 38 failures: 2 duration: 0.16s"Looks like you're just picking up extra text from the dom before comparing. |
| bottom = bottom - viewportRect.top; | ||
| left = left - viewportRect.left; |
There was a problem hiding this comment.
Could you fix the alignment on this line and below please?
* Right was calculated incorrectly (but was never used), which has been fixed. * The horizontal viewport check took the absolute value of the element's left, which didn't accurately check the viewport left/element right bound. This has been modified to check the element's left against the viewport's right (whether the element is within the right side of the viewport) and the element's right against the viewport's left (whether the element is within the left side of the viewport). * Adding a new test suite to test viewports of horizontally scrolling content. This verifies that when content is scrolled horizontally (e.g. a list of items), the isInViewport calculations are correct after scrolling.
|
@zeusdeux Sure, I think everything that you mentioned is fixed up. If there are any other problems let me know! |
Fix erroneous horizontal viewport check
|
Awesome, thanks for the merge! |
which didn't accurately check the viewport left/element right bound. This has
been modified to check the element's left against the viewport's right
(whether the element is within the right side of the viewport) and the
element's right against the viewport's left (whether the element is within
the left side of the viewport).