Accept an options object in scroll method
#39
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
CLAs look good, thanks! |
| @@ -189,15 +189,21 @@ | |||
| * Scrolls the content to a particular place. | |||
| * | |||
| * @method scroll | |||
| * @param {number} left The left position | |||
| * @param {number} top The top position | |||
| * @param {number|!ScrollToOptions} leftOrOptions The left position or scroll options | |||
There was a problem hiding this comment.
FYI (especially @aomarks) i dont think theres a way (yet, if ever) to specify overloads. so technically scroll(obj, 1) is valid according to this. not a lot we can do about that as jsdoc/closure types don't seem to have any such way
There was a problem hiding this comment.
Nope, pretty sure there's no way to describe that to Closure.
iron-scroll-target-behavior.html
Outdated
| window.scrollTo(left, top); | ||
| scroll: function(leftOrOptions, top) { | ||
| var opts = leftOrOptions; | ||
| if (typeof leftOrOptions !== 'object') { |
There was a problem hiding this comment.
Maybe unpack the options into a left/top var if present, instead of always allocating this object.
| * @param top The top position | ||
| */ | ||
| scroll(left: number, top: number): void; | ||
| scroll(leftOrOptions: number|ScrollToOptions, top?: number): void; |
There was a problem hiding this comment.
Is ScrollToOptions a type Closure already has?
There was a problem hiding this comment.
its a type the spec (and thus typescript) has, but im not too sure if closure knows of it or not. if it doesn't, it should :p
|
This looks good to me, @keanulee can you give it a look too? |
iron-scroll-target-behavior.html
Outdated
|
|
||
| if (typeof leftOrOptions === 'object') { | ||
| left = leftOrOptions.x; | ||
| top = leftOrOptions.y; |
There was a problem hiding this comment.
The first arg of type ScrollToOptions should contain left and top , not x and y
There was a problem hiding this comment.
so it is. no clue where i pulled x/y from, in my initial code of this it was left/top 🙄
|
@keanulee here you go. resolved that issue |
Part of polymer/gen-typescript-declarations#79.
An element mixing in this behaviour won't compile in TypeScript because our definition of the
scrollmethod incorrectly overrides the base one thatElementhas.To resolve this, i've added an options object to match the built in scroll method.
I also re-ran gen-tsd.
cc @aomarks