[webui] jquery code review #1917
Conversation
| @@ -207,7 +207,7 @@ function change_role(obj) { | |||
| complete: function () { | |||
| $('#' + type + '_spinner').hide(); | |||
| $('#' + type + '_table input').animate({opacity: 1}, 200); | |||
| $('#' + type + '_table input').removeAttr('disabled'); | |||
| $('#' + type + '_table input').prop('disabled', false); | |||
There was a problem hiding this comment.
this btw already fails in jquery 2.1
There was a problem hiding this comment.
What do you mean @coolo? that the prop method doesn't work in jquery 2.1? I have tried this at build.opensuse.org in the javascript console and it works...
There was a problem hiding this comment.
@coolo : I switched the .removeAttr(prop) to .prop(prop, false) per jQuery's recommendation. Can you give an example of this failing?
It is almost always a mistake to use .removeAttr( "checked" ) on a DOM element.
There was a problem hiding this comment.
I meant to say that removeAttr('disabled') already fails in jquery 2.1 - but build.oo uses jquery 1.11.
So 👍 for the change
There was a problem hiding this comment.
I happend to learn the hard way :)
os-autoinst/openQA@ac7fe8b#diff-9fb9b9a3ce0b13423f7eb2ba327405b4
There was a problem hiding this comment.
Ah thanks for the clarification. Also, TIL Bootstrap 3.x is not compatible with jQuery 3.x (twbs/bootstrap#16834)
There was a problem hiding this comment.
yeah, but till buildservice drops bento theme, bootstrap 19 is out ;)
|
Nice then. @avindra can you rebase and push again to see if the tests stop failing? If it doesn't stop then you need to find out why.... |
|
@bgeuken I have already done this many times before... But the failing test is in the webui suite, so I wanted him to verify if is because the changes added |
|
@mdeniz : Sure thing, will rebase + push to trigger tests when I have a chance. |
These are some changes to update jQuery usage to recommended standards / best practices per jQuery 3.0 (latest). More will be needed, but this is a start.
* .attr('disabled') and .removeAttr('disabled') -> .prop('disabled') per https://jquery.com/upgrade-guide/3.0/#breaking-change-removeattr-no-longer-sets-properties-to-false
* Replace $.each usage with Array.prototype.{forEach,map,reduce}, etc... These functions are natively supported in IE9+, and their callback signature is more chainable and usable than what jQuery went with.
* replace success(data) with success(meaningfulName) (should be done throughout codebase).
|
The test suite is failing again after rebasing and pushing: https://travis-ci.org/openSUSE/open-build-service/jobs/143567753 The following 3 tests are failing: Does anyone have any insight as to how my proposed changes are breaking these tests? I can't spot anything obvious. |
|
Not without having a closer look. |
|
@avindra are you going to do anything about this? |
|
@hennevogel : I haven't gotten time to look at the failing tests yet. I'll have another look this weekend |
|
@avindra awesome, thanks |
|
@avindra if you find the time to look into it please re-submit a PR. Thanks |
These are some changes to update jQuery usage to recommended standards / best practices per jQuery 3.0 (latest). More will be needed, but this is a start.
.attr('disabled')and.removeAttr('disabled')->.prop('disabled')per https://jquery.com/upgrade-guide/3.0/#breaking-change-removeattr-no-longer-sets-properties-to-false$.eachusage withArray.prototype.{forEach,map,reduce}, etc... These functions are natively supported in IE9+, and their callback signature is more chainable and usable than what jQuery went with.success(data)withsuccess(meaningfulName)(should be done throughout codebase).