Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(#32): add Element.scroll/To/By methods #54

Merged
merged 8 commits into from
Feb 24, 2017

Conversation

ghybs
Copy link
Contributor

@ghybs ghybs commented Feb 21, 2017

as per new draft spec version, as mentioned in issue #32
https://drafts.csswg.org/cssom-view/#extension-to-the-element-interface

This PR only adds Element.prototype.scrollBy, it does not address scrollTo nor anything else yet.

Also added a dedicated section on the index.html demo page for immediate test:
https://ghybs.github.io/smoothscroll/

Tested on Firefox 51, Chromium 53, Opera 43.

NOTE: it might be interesting to refactor Element.prototype.scrollIntoView call to smoothScroll using scrollableParent.scrollBy?

as mentioned in iamdustan#32, `scrollBy` method (and others) has been added to the Element interface as well by latest draft spec.
iamdustan#32
https://drafts.csswg.org/cssom-view/#extension-to-the-element-interface
This commit only adds `scrollBy`.
to test in different browers right away.
should allow refactoring `scrollBy` and `scrollIntoView` later on.
Would also enable fixing bad fallback for `scrollBy` (in case of bailout).
side effect is that this refactoring eliminates the bad fallback for `Element.scrollBy` (was using `Element.scrollIntoView`, which gives a totally different behavior), and re-uses simply the fallback within `Element.scroll`.
@ghybs
Copy link
Contributor Author

ghybs commented Feb 22, 2017

Now added Element.prototype.scroll and Element.prototype.scrollTo, which enabled fixing the fallback (bailout) of Element.prototype.scrollBy.

This should address the entire feature suggestion of #32.

@ghybs ghybs changed the title Feat(#32): add Element.scrollBy method Feat(#32): add Element.scroll/To/By methods Feb 22, 2017
@ghybs
Copy link
Contributor Author

ghybs commented Feb 22, 2017

Tested on:

  • Firefox 51
  • Firefox for Android 51
  • Firefox for iOS 6
  • Chromium 53 & 56
  • Chrome for Android 56
  • Chrome for iOS 56
  • Opera 43
  • Safari 10
  • iOS Safari 10

Copy link
Owner

@iamdustan iamdustan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Thanks @ghybs. One minor change and it looks good to me!


if (typeof arg0 === 'object') {
arg0.left += this.scrollLeft;
arg0.top += this.scrollTop;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn’t mutate incoming arguments. We’ll likely want to introduce a really small and simple assign method to handle this.

this.scroll(assign({}, arg0, {
  left: this.scrollLeft + arg0.left,
  top: this.scrollTop + arg0.top,
  behavior: arg0.behavior,
}));

@iamdustan iamdustan mentioned this pull request Feb 22, 2017
but use a new object instead.
Since only `left`, `top` and `behavior` object members are used (at least for now), we do not need to actually clone the full input parameter; just using those 3 members keeps the code simple.
with refactoring of `Element.prototype.scrollBy` so that it does not change the input argument / parameter.
@ghybs
Copy link
Contributor Author

ghybs commented Feb 22, 2017

Nice thought about changed input argument.
Here you go.

I did not implement a full assign function (I guess you wanted it to clone the input?), as it looked quite overkill for just a simple use with 3 object members (always the same keys) to take care of.
Please let me know if I missed something.

@iamdustan
Copy link
Owner

👍 If the spec changes and adds new features we’ll just need to update then.

Appreash!

@iamdustan iamdustan merged commit 19cc13f into iamdustan:master Feb 24, 2017
@ghybs ghybs deleted the elementScrollBy branch February 24, 2017 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants