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

Scrollable index should force Number() type before setting index in seekTo() #125

Closed
shanerooks opened this Issue Jul 22, 2010 · 1 comment

Comments

Projects
None yet
2 participants
@shanerooks

Had a problem which I discovered derived from the ambiguity of variable types in Javascript. I mentioned it in the Scrollable forums and was able to solve it myself. You can find that discussion here:

http://flowplayer.org/tools/forum/35/46212#post-46316

It related to me using a.attr( "href" ) as the source for the index value in a seekTo call. Even though the value was numeric ( i.e. 5 ), being href, it was cast as String. This caused seekTo() to cast the new index value as a String in the scrollable object, essentially fubarring the function of move(), next(), and prev(). What should have been 5 + 1 = 6, would instead become "5" + 1 = "51".

Admittedly, I should have cast the href as a number BEFORE passing it to seekTo, which was my eventual workaround, but the library should also assume that it might get inappropriately cast variables and make sure the Types of internal properties don't get changed.

Anyway, I hope that was clear. I didn't poke around too much in the code, but I would suggest adding something like

i = Number(i);

at the opening of the seekTo() declaration.

S.

@tipiirai

This comment has been minimized.

Show comment
Hide comment
@tipiirai

tipiirai Jul 23, 2010

Contributor

yes. good idea. the index is now forced to be numeric. here is the patch:

http://github.com/jquerytools/jquerytools/commit/6e6b29b54d2a7228df51c23798a8967190d760b7

Contributor

tipiirai commented Jul 23, 2010

yes. good idea. the index is now forced to be numeric. here is the patch:

http://github.com/jquerytools/jquerytools/commit/6e6b29b54d2a7228df51c23798a8967190d760b7

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment