Refactor #11
Refactor #11
Conversation
…ted and string as top left parameters
| * @returns {Boolean} | ||
| */ | ||
| function bailOut(x) { | ||
| if (typeof x !== 'object' || x.behavior === undefined || x.behavior === 'auto' ) { |
There was a problem hiding this comment.
Firefox falls back to original method if behavior is undefinedor auto.
|
Dont' hate me @iamdustan, I got excited... |
| // global frame variable to avoid collision | ||
| frame, | ||
| // global metric variables | ||
| startY, startX, endX, endY; |
There was a problem hiding this comment.
is it possible to have multiple scrollBy/scrollTo’s going at once? I guess we’re only supporting those 3 scrolling methods and not element-with-overflow-scrolling.
There was a problem hiding this comment.
Not on the window object, but it could be possible that browsers allow scrolling in the window and an element, but I would have to test. Either way, functionality was not extended or modified.
|
👏👏 this is awesome @jeremenichelli. Great work. |
|
|
||
| // TODO: look into polyfilling scroll-behavior: smooth css property |
There was a problem hiding this comment.
this TODO would still be a nice to have. I have what the perf implications of looking up a CSS property on every element is though.
We could maybe get away with it since we only support scrolling the entire document or Element.prototype.scrollIntoView.
First thought is that in either window.scrollTo/scrollBy method we look at the html and body tags for that css property and if it is set we return calling the same method with the scroll behavior set.
There was a problem hiding this comment.
Actually, by that point we could easily support it when Element.scrollBy support is added by checking for that behavior when called. No need to traverse the DOM looking for all of those elements. JIT property checks.
There was a problem hiding this comment.
Yeap, I think is nitpicky but it would a nice-to-have and not that hard to implement.
There was a problem hiding this comment.
No need for that right now. I’ll take this PR as is. I made another issue (#12) for that.
There was a problem hiding this comment.
Roger that 👍
|
Once the couple nit/typos are corrected I’ll |
|
@iamdustan fixed typos, some bug on scrollIntoView and increase |
Suggestion
See diff in unified view.
Refactor
List of things improved:
bailOutmethod to returntrueif behavior is auto or undefined and throw error if behavior is not supported as Firefox does in its implementation of the standardfindScrollableParentmethod to stop if node is invalid and avoid crashingleft other comments and explanations over the diff file
tested in Chrome 43