Skip to content
Permalink
Browse files

feat($anchorScroll): convert numeric hash targets to string

This allows `$anchorScroll(7)` to scroll to `<div id="7">` (although technically, the target ID is a
string, not a number).

Fixes #14680

Closes #15182
  • Loading branch information
mrLarbi authored and gkalpak committed Sep 23, 2016
1 parent 3253b55 commit 9062bae05c002934fe7bfd76043dcc3de9acfde6
Showing with 14 additions and 1 deletion.
  1. +2 −1 src/ng/anchorScroll.js
  2. +12 −0 test/ng/anchorScrollSpec.js
@@ -238,7 +238,8 @@ function $AnchorScrollProvider() {
}

function scroll(hash) {
hash = isString(hash) ? hash : $location.hash();
// Allow numeric hashes
hash = isString(hash) ? hash : isNumber(hash) ? hash.toString() : $location.hash();

This comment has been minimized.

Copy link
@mgol

mgol Sep 30, 2016

Member

Why don't we just always cast the input to string in that case?

This comment has been minimized.

Copy link
@mgol

mgol Sep 30, 2016

Member

Also, these nested ternaries are not very readable, could we disallow them?

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 30, 2016

Member

We shouldn't stringify everything (e.g. undefined, null). And stringifying accidentally passed non-string values (such as objects or arrays) doesn't seem useful. #14680 explains the usecase for numbers, which didn't sound unreasonable or uncommon.

(I like mildly nested ternaries 😇, but I am always up for consistency too 😃)

This comment has been minimized.

Copy link
@mgol

mgol Sep 30, 2016

Member

People might still get surprised if they use <div id="123456789123456789"></div>. Code $anchorScroll(123456789123456789) won't work but $anchorScroll('123456789123456789') will.

It'd be better if people used correct input types. ;)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 30, 2016

Member

If people have 123456789123456789 elements on their page, not scrolling to the 123456789123456789th element would be the least of their problems 😛
I agree that using the correct type is always the best approach.

var elm;

// empty hash, scroll to the top of the page
@@ -260,6 +260,18 @@ describe('$anchorScroll', function() {
addElements('id=top'),
callAnchorScroll('top'),
expectScrollingTo('id=top')));


it('should scroll to element with id "7" if present, with a given hash of type number', inject(
addElements('id=7'),
callAnchorScroll(7),
expectScrollingTo('id=7')));


it('should scroll to element with id "7" if present, with a given hash of type string', inject(
addElements('id=7'),
callAnchorScroll('7'),
expectScrollingTo('id=7')));
});
});

0 comments on commit 9062bae

Please sign in to comment.
You can’t perform that action at this time.