Fix broken onScroll handler after v1.8.6 #397
Conversation
After fixing deprecation message for onScroll, a typo was introduced by using `onScrollToY` instead of `onScrollY`. This PR resolves this problem by using the correct event handler and adds a basic test to ensure the event gets fired properly.
| table, | ||
| currentScrollY: 0, | ||
| onScroll() { | ||
| assert.ok(true, 'onScroll worked'); |
There was a problem hiding this comment.
nit-picky, but do we want to check for e.target.scrollTop = 300?
There was a problem hiding this comment.
The actual value was slightly different, probably because of row heights, table size, etc. Just wanted to check the event handler was properly called on scroll.
Should I improve the test to also check for position or is this test good enough?
There was a problem hiding this comment.
it's probably because 300 > than the total row heights combined, did you try with a smaller value (in my experience it matches).
that being said even something as assert.ok(e.target.scrollTop > 0) proves your point that it did scroll.
assert.equal(e.target.scrollTop, scrollTopAmount) verifies that it scrolled the requested amount as well as it scrolled.
if it's not too hard I would try to add it
There was a problem hiding this comment.
Done 😃 .
300 was indeed too much scrolling for this test, 50 does the trick.
|
thanks for this @eruizdechavez |
Instead of just testing event handler being called, also test the value of the actual scrolling.
|
Thanks. Will update when released |
After fixing deprecation message for onScroll, a typo was introduced by using
onScrollToYinstead ofonScrollY. This PR resolves this problem by using the correct event handler and adds a basic test to ensure the event gets fired properly.