feat($anchorScroll): allow scrolling to a specified element #9596

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@gkalpak
Member
gkalpak commented Oct 13, 2014

Add an optional argument to $anchorScroll() to enable scrolling to an anchor element different than that related to the current value of $location.hash(). If the argument is omitted, the value of $location.hash() will be used instead.

Closes #4568

@mary-poppins mary-poppins added cla: yes and removed cla: no labels Oct 13, 2014
@tbosch tbosch self-assigned this Oct 13, 2014
@tbosch tbosch modified the milestone: low, Backlog Oct 13, 2014
@tbosch tbosch removed their assignment Oct 13, 2014
@gkalpak
Member
gkalpak commented Nov 10, 2014

Small and safe little feature, with docs and tests - what's not to love about this PR (even Travis likes it) ;)

@googlebot

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added cla: no and removed cla: yes labels Nov 21, 2014
@googlebot

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Nov 23, 2014
@petebacondarwin petebacondarwin and 1 other commented on an outdated diff Apr 2, 2015
test/ng/anchorScrollSpec.js
return function($anchorScroll) {
- $anchorScroll();
+ $anchorScroll.apply(null, args);
@petebacondarwin
petebacondarwin Apr 2, 2015 Member

Why does this need to be so complicated? Could it not simply be:

function callAnchorScroll(hash) {
  return function($anchorScroll) {
    $anchorScroll(hash);
  };
}
@gkalpak
gkalpak Apr 2, 2015 Member

Hm...why indeed ?

@petebacondarwin petebacondarwin and 1 other commented on an outdated diff Apr 2, 2015
src/ng/anchorScroll.js
@@ -232,8 +236,9 @@ function $AnchorScrollProvider() {
}
}
- function scroll() {
- var hash = $location.hash(), elm;
+ function scroll(hash) {
+ hash = (arguments.length && isString(hash)) ? hash : $location.hash();
@petebacondarwin
petebacondarwin Apr 2, 2015 Member

Can we not just check whether hash is undefined?

@gkalpak
gkalpak Apr 2, 2015 Member

That would be the reasonable thing to do :)
I was trying to avoid accidentally breaking an existing app, by ensuring that the hash argument is a string.
It's been a while, so I am not really sure about the arguments.length check; I suspect it was me micro-optimizing (or at least believing so).

It seems that isString(hash) ? hash : $location.hash() would be enough.

I would rather use isString() instead of isDefined(), to avoid breaking some unlikely construct like:

elem.on('click', $anchorScroll);   // e.g. passing and event object will be ignored
@petebacondarwin
Member

I would make the commit title: feat($anchorScroll): allow scrolling to a specified element

@petebacondarwin
Member

If you could deal with the small comments above then LGTM

@gkalpak gkalpak changed the title from feat($anchorScroll): add support for scrolling independently of `$location.hash()` to feat($anchorScroll): allow scrolling to a specified element Apr 2, 2015
@petebacondarwin
Member

LGTM

@gkalpak gkalpak feat($anchorScroll): allow scrolling to a specified element
Add an optional argument to `$anchorScroll()` to enable scrolling to an
anchor element different than that related to the current value of
`$location.hash()`. If the argument is omitted or is not a string,
the value of `$location.hash()` will be used instead.

Closes #4568
55f44b7
@gkalpak gkalpak added a commit that closed this pull request Apr 2, 2015
@gkalpak gkalpak feat($anchorScroll): allow scrolling to a specified element
Add an optional argument to `$anchorScroll()` to enable scrolling to an
anchor element different than that related to the current value of
`$location.hash()`. If the argument is omitted or is not a string,
the value of `$location.hash()` will be used instead.

Closes #4568
Closes #9596
731c8b5
@gkalpak gkalpak closed this in 731c8b5 Apr 2, 2015
@gkalpak gkalpak deleted the gkalpak:$anchorScroll-explicit-hash branch Apr 2, 2015
@netman92 netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@gkalpak @netman92 gkalpak + netman92 feat($anchorScroll): allow scrolling to a specified element
Add an optional argument to `$anchorScroll()` to enable scrolling to an
anchor element different than that related to the current value of
`$location.hash()`. If the argument is omitted or is not a string,
the value of `$location.hash()` will be used instead.

Closes #4568
Closes #9596
7e91cc5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment