changed debounce out for bindRaf #14
Conversation
pull latest from main repo
the debounce that was setup for calling on scroll/resize was 10ms. All browsers that I know of set a FPS cap of 60fps. 1s/60f = 16ms. So for two reasons I have changed to bindRaf 1. There is no reason to check if we are below the desired point to change the element to visable until we are rendering a new frame, now if the fps tanks to say 25fps the calculations will only call once every 40ms, so 1/4 the calcs when the system is struggling. 2. setTimeout is not guarinteed to call on the time specified. See https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/setTimeout#Reasons_for_delays_longer_than_specified I came to the conclution that this was required after reading this SOF answer: https://stackoverflow.com/questions/41740082/scroll-events-requestanimationframe-vs-requestidlecallback-vs-passive-event-lis Cheers.
src/lib/bindRaf.js
Outdated
|
|
||
| return () => { | ||
| that = this; | ||
| args = arguments; |
There was a problem hiding this comment.
not using these variables, so could probably make this simpler.
There was a problem hiding this comment.
Will try to get around to fixin that tomorrow, sorry I tried a few solution before I was happy, that’s the left over mess
There was a problem hiding this comment.
@dazld Just read over the code. both variables are used.
- self is used so that on callback the context of the function we are wrapping has the same context as the place we bound it (I think)
- args is used to grab the arguments that were passed in upon creation of the callback.
I have made another commit to rename that to the more standard self name and removed debounce as it's unused.
The only improvement this could use would be a cancellation token, but I think that can wait as the not binding to null fix previously is good enough.
Any more issues?
There was a problem hiding this comment.
I should learn to read, sorry @falconmick - no, this all looks ok.
|
|
||
| # for now removing package-lock as it's not my decision to include | ||
| package-lock.json |
There was a problem hiding this comment.
good point, we should pin this somehow.
| @@ -2,12 +2,12 @@ | |||
| import React, { Component } from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import cx from 'classnames'; | |||
| import debounce from './lib/debounce'; | |||
| import { bindRaf } from "./lib/bindRaf"; | |||
There was a problem hiding this comment.
could you use kebab case for file names?
| @@ -1,9 +0,0 @@ | |||
| export default function debounce(fn, time) { | |||
There was a problem hiding this comment.
could probably use this as a fallback for browsers that don't have rAF, so maybe keep it around.
|
tested it locally, seems good to go! |
|
published as |
|
Thanks
…On Tue, 14 Nov 2017 at 5:07 AM, Dan Peddle ***@***.***> wrote:
published as 1.3.0!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAri6STZ40J01j4IO2rlfFZxqBUdOjlhks5s2K-kgaJpZM4Pumli>
.
|
the debounce that was setup for calling on scroll/resize was 10ms. All browsers that I know of set a FPS cap of 60fps. 1s/60f = 16ms.
So for two reasons I have changed to bindRaf
I came to the conclution that this was required after reading this SOF answer: https://stackoverflow.com/questions/41740082/scroll-events-requestanimationframe-vs-requestidlecallback-vs-passive-event-lis
Also I have not followed your current standards for code, instead of functions I've used ES6 syntax aswell as not using default exports on library functions. If you don't like this feel free to change it around, but this is how I coded it when I was looking for a solution to my problem.
Cheers.