Tests: move readywait to an iframe test #3331
Conversation
|
What you trying to fix? |
|
I see – LGTM |
| // has been decremented by readywaitloader.js | ||
| // If an error occurs. | ||
| jQuery("#output").append(delayedMessage); | ||
| startIframeTest( delayedMessage ); |
There was a problem hiding this comment.
This is a bit self-fulfilling, since delayedMessage will always be "It worked!". I'd rather see this send a global variable that is assigned in the same callback that releases the ready hold.
|
|
||
| setTimeout( function() { | ||
| jQuery.holdReady( false ); | ||
| }, 2000 ); |
There was a problem hiding this comment.
Two seconds is a lot of idle time to add for this test. If you take my advice below about assigning to a variable here, we can instead set a short timeout and just push it back if it fires before jQuery.ready has been invoked.
| "unit/basic.js", | ||
|
|
||
| "unit/core.js", | ||
| "unit/readywait.js", |
There was a problem hiding this comment.
Adding a new module means more than extending this list. Can you instead put the new test in test/unit/ready.js?
|
Thanks @gibson042 for your review. I did think that the change wasn't ideal but I tried just to do minimum changes. Will fix it soon 😄 |
|
Hey @stevemao. If you have time to make the requested changes, we'd like to land this for 3.2. |
|
Closing in favor of #3576 |
7 participants
Summary
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.See gh-3285