Event: fixing incorrect window bug with scrollTop/Left in iframes #1959
Conversation
|
💛 |
| @@ -531,4 +531,19 @@ test("fractions (see #7730 and #7885)", function() { | |||
| div.remove(); | |||
| }); | |||
|
|
|||
| test("iframe scrollTop/Left (see #1945)", function() { | |||
| expect( 2 ); | |||
There was a problem hiding this comment.
This could be moved to test declaration, like so:
test("iframe scrollTop/Left (see #1945)", 2, function() {
There was a problem hiding this comment.
That's not recommended: qunitjs/qunit#356
There was a problem hiding this comment.
Not yet, but that is the news either way.
@jzaefferer, @Krinkle is that still something your planning to do?
There was a problem hiding this comment.
Regardless of what's the resolution of qunitjs/qunit#356, I agree with this comment from @jzaefferer: using expect makes the intention explicit so I like it better.
There was a problem hiding this comment.
Let's not fixate on subjective opinions, everybody has one whereas those discussions are always exhausting.
There was a problem hiding this comment.
For this PR, then, can we agree to leave it as is?
There was a problem hiding this comment.
Yep, but i still would like to hear someone from qunit team
There was a problem hiding this comment.
Yes, that ticket should've been closed as fixed, along with follow up tickets for removal. See also http://qunitjs.com/upgrade-guide-2.x/
In this case, keep calling expect, and switch to assert.expect some time.
There was a problem hiding this comment.
Aim for consistency with the surrounding code first and foremost. We'll switch to something when we switch to it, in a dedicated commit.
There was a problem hiding this comment.
@Krinkle i'd would appreciate, if you comment only if you have something to say i.e. without meaningless tautology
araghava
force-pushed
the
fix-scroll-reset-bug
branch
from
4f35f25 to
3fd7134
Compare
|
This LGTM as is, I think we want to stay with |
|
@araghava could sign your commit with your real name? After that, we will able to land this. |
araghava
force-pushed
the
fix-scroll-reset-bug
branch
from
3fd7134 to
25c7c6f
Compare
|
@markelog signed with real name |
|
@dmethvin hm, just realized, this commit has "Event" as a module for some reason |
|
Right sorry this should have been under 'Offset:' I believe. |
The iframe was getting the parent window's value for pageXOffset and pageYOffset, see #1945.