BorisMoore / jsviews

Interactive data-driven views, MVVM and MVP, built on top of JsRender templates
http://www.jsviews.com/#jsviews
MIT License
856 stars 130 forks source link

jquery-ui radio tags in a group are loosing their checked state when enabled/disabled #373

Closed trueqbit closed 7 years ago

trueqbit commented 7 years ago

Hi Boris,

whenever a {^{radio/}} jquery-ui tag control is contained in a {^{radiogroup}} and it's 'disabled state' is data-linked, then the jsviews-jqueryui library removes the 'checked state' from all radio buttons in this group. Note that free-standing radio buttons don't exhibit this issue, so it must be something with the {^{radiogroup}} tag.

I am not familiar with how everything is implemented - I could only investigate the callstack and noticed that checkboxRadioOnAfterLink() extracts the value bound as argument to the tag, but the value is undefined.

Here's a fiddle that shows what I mean: https://jsfiddle.net/pieceofcode/td8zsfzz/

Another strange thing is that the proper initial 'checked state' depends on whether jquery-ui.css gets loaded or not.

BorisMoore commented 7 years ago

Thanks Klaus, I'll look into it...

BorisMoore commented 7 years ago

Klaus, I think the following updated version of jsviews-jqueryui-widgets.js should fix those issues. Can you test it in your environment, and let me know? Thanks.

jsviews-jqueryui-widgets.zip

trueqbit commented 7 years ago

Boris, you are awesome! It's working as expected.

BorisMoore commented 7 years ago

Good to hear. Thanks for calling it out!

I'll publish the fix along with 0.9.85. Meantime you can use this patched version...

BorisMoore commented 7 years ago

Here is a zip that includes a minified version.

jsviews-jqueryui-widgets.zip

trueqbit commented 7 years ago

Your change broke the checkbox. See this diff for the fix I propose.

BorisMoore commented 7 years ago

Yes, totally. I rushed out that patch to you for your scenario too fast, I guess, before completing the testing and verifying... Of course, as you show, it was missing the corresponding fix for checkbox.

Thanks for calling this out. The proposed diff looks right to me...

BorisMoore commented 7 years ago

I might write it this way, for consistency and size:

function checkboxRadioOnSetValue(val) {
  if (val !== undefined) {
    var elem = this.mainElem[0];
    elem.checked = elem.type === "radio"
      ? (val === elem.value)
      : val && val !== "false";
  }
  this.widget.refresh();
}
checkbox: {
  ...
  onAfterLink: checkboxRadioOnAfterLink,
  setValue: checkboxRadioOnSetValue
},
radio: {
  ...
  onAfterLink: checkboxRadioOnAfterLink,
  setValue: checkboxRadioOnSetValue
},

Though on second thoughts, maybe the separate implementations is better since I'm not sure we need widget.refresh() on the checkbox. Did you include it because you saw a need for it? We may be able to write simply:

  setValue: function(val) {
    if (val !== undefined) {
      var elem = this.mainElem[0];
      elem.checked = val && val !== "false";
    }
  }

for the checkbox.

trueqbit commented 7 years ago

Well, the separate implementations would make sense because we don't need to check for the element type, right? But I am happy with a common method as well, since they are sharing the other already.

jquery's checkboxradio widget has a refresh() method, so it would be necessary to call it also for the checkbox? I was basically taking code from the radio tag control, which calls it.

Side-note: BTW, it would be even useful to have those functions available outside of the jsviews-jqueryui-widgets.js file, when somebody (like me) wants to have e.g. a sliding checkbox, otherwise I have to duplicate code (Or I probably didn't come yet to the part where deriving widgets becomes useful).

BorisMoore commented 7 years ago

For the radio tag the call to widget.refresh() is necessary, since there is a set of radio button widgets, and calling refresh makes the group of widgets take into account the modified 'checked' state of the one that got set.

The checkbox is different - and doesn't need to be refreshed. (Sure the checkbox has a refresh method, but we don't need to call it.)

BorisMoore commented 7 years ago

The discussed changes/fix are included in commit 85.