fix(Visibility): handle context scroll instead of just window scroll #3400
Conversation
|
💖 Thanks for opening this pull request! 💖 Here is a list of things that will help get it across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
Codecov Report
@@ Coverage Diff @@
## master #3400 +/- ##
==========================================
+ Coverage 99.81% 99.81% +<.01%
==========================================
Files 174 174
Lines 2725 2730 +5
==========================================
+ Hits 2720 2725 +5
Misses 5 5
Continue to review full report at Codecov.
|
| // onUpdate.should.have.been.calledWithMatch(null, { | ||
| // calculations: { direction: 'down' }, | ||
| // }) | ||
| // }) |
There was a problem hiding this comment.
Does this test fail? Per the PR description, I've marked this one as needing some help! 👍
|
Looking good so far! As noted, we just need some more hands on the test case. |
|
Thanks for the feedback @levithomason |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
@layershifter Thank you very much for your help with this PR. |
|
Only Levi has release rights and he is on a vacation right now. I have asked him for a release, but I don't have any ETA 🙂 |
|
No problem, thanks :) |
…emantic-Org#3400) * fix(Visibility): handle context scroll instead of just window scroll * fix(Visibility): refactor and uncomment failing test * add new test, fix broken behavior * add note * remove comment * Update Visibility.js * Update Visibility.js
Currently adding a context to the Visibility component will add event listeners for that context, however the inner workings of the Visibility component still use the window scroll position for calculations.
This means that we are unable to use the Visibility component on any context other than the window.
This PR seeks to resolve this issue.
The code changes I have made are working for my use case, however I require some assistance in getting the tests to work as well as ensuring that the changes I have made are suitable for all use cases of the Visibility component.
Thanks :)