More efficient scroll function #39
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I just tried using this plugin for the first time, so I don't know the ins and outs necessarily. But the `scroll` event triggers a *lot* when scrolling, and in this case the `detectTarget` function that is bound to the `scroll` event is somewhat inefficient.
Without actually changing anything in the way the plugin or that function work, we can make it much more efficient by using plain JavaScript where possible.
In this case, jQuery is definitely not needed to get an element's `id`. Also, as far as I can tell `scope` will always be a jQuery object already because the plugin is called via the jQuery selector (e.g. `$('#my-element').endlessScroll()`.
So, 1) we don't need to re-wrap `$(this.target)`, since `this.target` is always a jQuery object already (again, I could be wrong here, I don't know the ins and outs of the plugin), and 2) we can get the plain JS dom element using `.get(0)` and then just call the plain JS `id` property on it; much more efficient.
Ideally though, this function should probably just return true if `this.target` and `this.targetId` are already present, but that actually changes the way the function works and could mess up on pages that are constantly having their DOM updated and changed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I just tried using this plugin for the first time, so I don't know the ins and outs necessarily. But the
scrollevent triggers a lot when scrolling, and in this case thedetectTargetfunction that is bound to thescrollevent is somewhat inefficient.Without actually changing anything in the way the plugin or that function work, we can make it much more efficient by using plain JavaScript where possible.
In this case, jQuery is definitely not needed to get an element's
id. Also, as far as I can tellscopewill always be a jQuery object already because the plugin is called via the jQuery selector (e.g.$('#my-element').endlessScroll().So, 1) we don't need to re-wrap
$(this.target), sincethis.targetis always a jQuery object already (again, I could be wrong here, I don't know the ins and outs of the plugin), and 2) we can get the plain JS dom element using.get(0)and then just call the plain JSidproperty on it; much more efficient.Ideally though, this function should probably just return true if
this.targetandthis.targetIdare already present, but that actually changes the way the function works and could mess up on pages that are constantly having their DOM updated and changed.