Make spinbox trigger changed after setValue #1960
Conversation
|
@vargasd please look into the failing unit test and ensure they are all passing. |
|
Should I open another PR to fix the failing unit tests? They were previously broken and don't relate to this issue. |
| $spinbox.spinbox('setValue', '2'); | ||
|
|
||
| $spinbox.on('changed.fu.spinbox', function () { | ||
| assert.ok(true, 'spinbox triggers changed after input' ); |
There was a problem hiding this comment.
This is probably not even getting validated. You need to use assert.async in here and call it to ensure this assertion gets asserted. This is an async test, so you need to tell QUnit it is async.
There was a problem hiding this comment.
This isn't asynchronous. The handler should be called synchronously on the focusout. I can make it asynchronous, but then the failure would happen because of a timeout rather than a 0-assertion test. I prefer the latter because it means the tests run more quickly, but I can change it if you want.
There was a problem hiding this comment.
this is fine for now but for future reference when using an event listener an async test is more appropriately because it is unknown what is happening before the event is triggered.
|
I didn't know the unit tests were previous broken. You don't need to open a PR, @swilliamset can look into those. I just wanted to double check they weren't from your code changes is all. |
|
unit tests are failing on repeater tests the same as #1959. i'm looking into it. once i have unit tests resolved and merged in the PR with a fix you can rebase this PR. |
After programmatically setting the value of the spinbox, the previous
value would be retained in the lastValue property of the control,
causing the "changed" event not to get triggered if the user were to
enter the previous value again. To avoid this, the current setValue
method was converted into a private method with an additional parameter
to set the last value, and the public method will wrap this and pass
that parameter, while internal uses of the method will use the private
method to avoid other side effects.
Fixes #1950
|
Hey @swilliamset, I have rebased and tests are now passing. |
|
@swilliamset Now that Sam has rebased and the tests are passing, is there anything else you need to complete the review? |
|
i'll take a look at this on Monday. |
|
@swilliamset any update on this review? Thanks. |
|
My apologies for the delay. This is priority 0 today. |
| $spinbox.spinbox('setValue', '2'); | ||
|
|
||
| $spinbox.on('changed.fu.spinbox', function () { | ||
| assert.ok(true, 'spinbox triggers changed after input' ); |
There was a problem hiding this comment.
this is fine for now but for future reference when using an event listener an async test is more appropriately because it is unknown what is happening before the event is triggered.
After programmatically setting the value of the spinbox, the previous
value would be retained in the lastValue property of the control,
causing the "changed" event not to get triggered if the user were to
enter the previous value again. To avoid this, the current setValue
method was converted into a private method with an additional parameter
to set the last value, and the public method will wrap this and pass
that parameter, while internal uses of the method will use the private
method to avoid other side effects.
Fixes #1950