-
-
Notifications
You must be signed in to change notification settings - Fork 14
changed debounce out for bindRaf #14
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,10 @@ | ||
node_modules | ||
build | ||
npm-debug.log | ||
bundle.js | ||
bundle.js | ||
|
||
# ide folders | ||
.idea | ||
|
||
# for now removing package-lock as it's not my decision to include | ||
package-lock.json | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you use kebab case for file names? |
||
|
||
class OnVisible extends Component { | ||
constructor() { | ||
super(...arguments); | ||
this.onScroll = debounce(this.onScroll.bind(this), 10); | ||
this.onScroll = bindRaf(this.onScroll.bind(this)); | ||
this.state = { | ||
visible: false, | ||
bottom: 0, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
export const bindRaf = (fn) => { | ||
let isRunning = null; | ||
let that = null; | ||
let args = null; | ||
|
||
const run = () => { | ||
isRunning = false; | ||
fn.apply(that, args); | ||
}; | ||
|
||
return () => { | ||
that = this; | ||
args = arguments; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not using these variables, so could probably make this simpler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dazld Just read over the code. both variables are used.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should learn to read, sorry @falconmick - no, this all looks ok. |
||
|
||
if (isRunning) { | ||
return; | ||
} | ||
|
||
isRunning = true; | ||
requestAnimationFrame(run); | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, we should pin this somehow.