no jquery fixes #123
no jquery fixes #123
Conversation
No jquery fixes
No jquery
| } else if (entry.intersectionRatio <= 0) { // exiting viewport | ||
| set(this, 'viewportEntered', false); | ||
| this.trigger('didExitViewport'); | ||
| if (!this.isDestroyed && !this.isDestroying) { |
There was a problem hiding this comment.
This is a good catch! I’m guessing you saw this error when trying out this branch?
There was a problem hiding this comment.
I see this error now. Nice
| _triggerDidScrollDirectionHandler(event) { | ||
| const sensitivity = get(this, 'viewportScrollSensitivity') || 1; | ||
|
|
||
| this._debouncedEventHandler('_triggerDidScrollDirection', event.currentTarget, sensitivity); |
There was a problem hiding this comment.
Curious what your thoughts are bw ever.currentTarget vs scrollableArea vs window in detecting scroll direction. Perhaps irrelevant thus this might be a good fix?
There was a problem hiding this comment.
scrollableArea property may be null if it's not set in the component where the mixin is used. So I believe currentTarget is the best idea here.
There was a problem hiding this comment.
@latata If scrollableArea is not defined, then it defaults to window. Did you happen to see in error wrt to this?
| @@ -37,6 +37,8 @@ export default Mixin.create({ | |||
| }, this._buildOptions()); | |||
|
|
|||
| setProperties(this, options); | |||
| this._triggerDidScrollDirectionHandler = this._triggerDidScrollDirectionHandler.bind(this); | |||
| this._setViewportEnteredHandler = this._setViewportEnteredHandler.bind(this); | |||
There was a problem hiding this comment.
This somewhat spreads out the logic wrt to setViewportEntered. What do ou think?
There was a problem hiding this comment.
do you have any better idea how to remove handlers?
Unobserve intersection observer
| _triggerDidScrollDirectionHandler(event) { | ||
| const sensitivity = get(this, 'viewportScrollSensitivity') || 1; | ||
|
|
||
| this._debouncedEventHandler('_triggerDidScrollDirection', event.currentTarget, sensitivity); |
There was a problem hiding this comment.
@latata If scrollableArea is not defined, then it defaults to window. Did you happen to see in error wrt to this?
snewcomer
force-pushed
the
upgrade
branch
2 times, most recently
from
c20e9b5
to
348e0f9
Compare
|
@latata can you please rebase this branch? Thank you. |
|
Closing this for now. There were some good ideas here so thank you for that 🙏 . Following up in #138 |
Fixes