New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Styling of added checkbox incorrect when in controlgroup #3496

Closed
offsky opened this Issue Jan 26, 2012 · 19 comments

Comments

Projects
None yet
@offsky

offsky commented Jan 26, 2012

If you programmatically add a new checkbox to a controlgroup of existing checkboxes, the styling is incorrect. It does not merge the borders with the adjacent checkboxes.

Example: http://jsfiddle.net/JwWpX/1/

@samdz

This comment has been minimized.

Show comment
Hide comment
@samdz

samdz Jan 31, 2012

you are correct. I have tried your code in jsfiddle. It does not work. Not even with the 'trigger' or 'page' methods

samdz commented Jan 31, 2012

you are correct. I have tried your code in jsfiddle. It does not work. Not even with the 'trigger' or 'page' methods

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch Feb 3, 2012

Contributor

Yes, here is a Gist to reproduce this (with a bookmarklet to run it):
https://gist.github.com/1729775

You need to trigger the "create" event, that's missing in the example above. But even if you do so (like in my example), the old checkboxes still have the ui-corner-bottom css class.

Tobias

Contributor

tbosch commented Feb 3, 2012

Yes, here is a Gist to reproduce this (with a bookmarklet to run it):
https://gist.github.com/1729775

You need to trigger the "create" event, that's missing in the example above. But even if you do so (like in my example), the old checkboxes still have the ui-corner-bottom css class.

Tobias

@samdz

This comment has been minimized.

Show comment
Hide comment
@samdz

samdz Feb 3, 2012

I think this is still an outstanding bug that needs to be fixed. I tried you code, even adding

$("#page").page("destroy").page();

It does not work in Firefox, safari, chrome or IE.

samdz commented Feb 3, 2012

I think this is still an outstanding bug that needs to be fixed. I tried you code, even adding

$("#page").page("destroy").page();

It does not work in Firefox, safari, chrome or IE.

@klemensz

This comment has been minimized.

Show comment
Hide comment
@klemensz

klemensz Apr 24, 2012

Same problem here. Will this be fixed or is there a workaround?

klemensz commented Apr 24, 2012

Same problem here. Will this be fixed or is there a workaround?

@toddparker

This comment has been minimized.

Show comment
Hide comment
@toddparker

toddparker May 5, 2012

Contributor

@tigbro - Is this a bug in jQM or Angular? Can't quite tell from this thread. If it's on our end, could you post a test page using latest? Template using latest here: http://jsbin.com/agewuy/edit

Contributor

toddparker commented May 5, 2012

@tigbro - Is this a bug in jQM or Angular? Can't quite tell from this thread. If it's on our end, could you post a test page using latest? Template using latest here: http://jsbin.com/agewuy/edit

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch May 5, 2012

Contributor

Hi,
sorry, about my last comment. I confused the projects (thought that this tracker was part of my jqm angular adapter...), so I deleted my last comment.

Anyway, the issue is in jquery mobile, not in angular nor in my adapter.
Here is another jsfiddle to reproduce this (sorry, somehow the jsbin template did not work, it always showed the jqm wait icon...):
http://jsfiddle.net/hDc38/

As far as I can see, the problem is the following:
$.fn.controlgroup does not remove the style classes ui-corner-left, ui-corner-right, ui-corner-top, ui-corner-bottom.

By the way, it would be great to turn $.fn.controlgroup into a real jqm widget, just like collapsibleset or listview, with a refresh function. Same would be cool to have for $.fn.grid. This would help creating single page apps with jquery mobile that use those components.

Tobias

Contributor

tbosch commented May 5, 2012

Hi,
sorry, about my last comment. I confused the projects (thought that this tracker was part of my jqm angular adapter...), so I deleted my last comment.

Anyway, the issue is in jquery mobile, not in angular nor in my adapter.
Here is another jsfiddle to reproduce this (sorry, somehow the jsbin template did not work, it always showed the jqm wait icon...):
http://jsfiddle.net/hDc38/

As far as I can see, the problem is the following:
$.fn.controlgroup does not remove the style classes ui-corner-left, ui-corner-right, ui-corner-top, ui-corner-bottom.

By the way, it would be great to turn $.fn.controlgroup into a real jqm widget, just like collapsibleset or listview, with a refresh function. Same would be cool to have for $.fn.grid. This would help creating single page apps with jquery mobile that use those components.

Tobias

@toddparker

This comment has been minimized.

Show comment
Hide comment
@toddparker

toddparker May 5, 2012

Contributor

Thanks @tigbro. Mind creating issues for making thoe into real widgets if there isn' already one? I agree we should give that a look.

Contributor

toddparker commented May 5, 2012

Thanks @tigbro. Mind creating issues for making thoe into real widgets if there isn' already one? I agree we should give that a look.

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot May 5, 2012

Member

hi @tigbro

You are right about the cause of the problem and that means it might only require a minor change to make this work.

By adding the classes you mentioned (and ui-controlgroup-last) to the list of classes that have to be removed we can use the current controlgroup function to refresh a controlgroup as well.
Only difference with real widgets would be that you don't have to send the 'refresh' parameter: $(#mycontrolgroup).controlgroup(); This is something we would have to add to the info in the docs.

Here is a fiddle that loads a version of the JS in which I made the change: http://jsbin.com/agayas/11/
Of course it requires some more testing and thinking about edge cases but so far I don't see a problem.

I don't think controlgroup needs to become a 'real' widget because it is always used in combination with another widget (button, checkboxradio or selectmenu). So it is more like an extension of those.
Maybe we can make it work this way that when you refresh those widgets the framework checks if they are wrapped in a controlgroup and, if so, trigger controlgroup() on that element.

I am in favour of a grid widget! It would be great when the grid/block classes are added by the framework like it is done for navbars so you can dynamically inject a new block.

@offsky
In your fiddle you use document ready. In the docs at the page about events you can read why you shouldn't use that. Also, you call .checkboxradio() after appending new button markup. That function is to refresh the button after you manipulated the state of it. When injecting one you should use .trigger( "create");

[Update: changed link to new version of fiddle after relocating JS file]

Member

jaspermdegroot commented May 5, 2012

hi @tigbro

You are right about the cause of the problem and that means it might only require a minor change to make this work.

By adding the classes you mentioned (and ui-controlgroup-last) to the list of classes that have to be removed we can use the current controlgroup function to refresh a controlgroup as well.
Only difference with real widgets would be that you don't have to send the 'refresh' parameter: $(#mycontrolgroup).controlgroup(); This is something we would have to add to the info in the docs.

Here is a fiddle that loads a version of the JS in which I made the change: http://jsbin.com/agayas/11/
Of course it requires some more testing and thinking about edge cases but so far I don't see a problem.

I don't think controlgroup needs to become a 'real' widget because it is always used in combination with another widget (button, checkboxradio or selectmenu). So it is more like an extension of those.
Maybe we can make it work this way that when you refresh those widgets the framework checks if they are wrapped in a controlgroup and, if so, trigger controlgroup() on that element.

I am in favour of a grid widget! It would be great when the grid/block classes are added by the framework like it is done for navbars so you can dynamically inject a new block.

@offsky
In your fiddle you use document ready. In the docs at the page about events you can read why you shouldn't use that. Also, you call .checkboxradio() after appending new button markup. That function is to refresh the button after you manipulated the state of it. When injecting one you should use .trigger( "create");

[Update: changed link to new version of fiddle after relocating JS file]

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot May 6, 2012

Member

This line in my previous comment makes no sense: "Maybe we can make it work this way that when you refresh those widgets the framework checks if they are wrapped in a controlgroup and, if so, trigger controlgroup() on that element." Sorry.

Refreshing a button, checkboxradio or selectmenu means updating its state. No reason to refresh the controlgroup corner styles after that. It would only apply after you used trigger("create") to inject a new button. So we could look into a way to make that event trigger the controlgroup function on the parent element with data-role controlgroup

Member

jaspermdegroot commented May 6, 2012

This line in my previous comment makes no sense: "Maybe we can make it work this way that when you refresh those widgets the framework checks if they are wrapped in a controlgroup and, if so, trigger controlgroup() on that element." Sorry.

Refreshing a button, checkboxradio or selectmenu means updating its state. No reason to refresh the controlgroup corner styles after that. It would only apply after you used trigger("create") to inject a new button. So we could look into a way to make that event trigger the controlgroup function on the parent element with data-role controlgroup

@scottjehl scottjehl closed this in 2bab321 May 15, 2012

gseguin pushed a commit to gseguin/jquery-mobile that referenced this issue Jun 27, 2012

Merge pull request jquery#4299 from uGoMobi/issue_#3496
Fixes jquery#3496 - Styling of added checkbox incorrect when in controlgroup
@klemensz

This comment has been minimized.

Show comment
Hide comment
@klemensz

klemensz Aug 2, 2012

Is this supposed to be resolved? I still get the same effect with 1.1.1.

klemensz commented Aug 2, 2012

Is this supposed to be resolved? I still get the same effect with 1.1.1.

@deakjahn

This comment has been minimized.

Show comment
Hide comment
@deakjahn

deakjahn Oct 11, 2012

Not particularly nice but it works:

function workaround($controlgroup) {
  $controlgroup
    .controlgroup()
    .trigger("create")
    .find("label, label .ui-btn-inner")
      .removeClass("ui-btn-corner-all ui-corner-top ui-corner-bottom ui-corner-left ui-corner-right ui-controlgroup-last ui-shadow")
    .end()
    .find("label:first, label:first .ui-btn-inner")
      .addClass("ui-corner-top ui-controlgroup-first ui-shadow")
    .end()
    .find("label:last, label:last .ui-btn-inner")
      .addClass("ui-corner-bottom ui-controlgroup-last ui-shadow");
}

To be called after dynamic modification of the control group.

deakjahn commented Oct 11, 2012

Not particularly nice but it works:

function workaround($controlgroup) {
  $controlgroup
    .controlgroup()
    .trigger("create")
    .find("label, label .ui-btn-inner")
      .removeClass("ui-btn-corner-all ui-corner-top ui-corner-bottom ui-corner-left ui-corner-right ui-controlgroup-last ui-shadow")
    .end()
    .find("label:first, label:first .ui-btn-inner")
      .addClass("ui-corner-top ui-controlgroup-first ui-shadow")
    .end()
    .find("label:last, label:last .ui-btn-inner")
      .addClass("ui-corner-bottom ui-controlgroup-last ui-shadow");
}

To be called after dynamic modification of the control group.

@adaptabi

This comment has been minimized.

Show comment
Hide comment
@adaptabi

adaptabi Oct 31, 2012

Contributor

Bummer, this bug still exists in 1.2 Any news when it will get fixed??

Contributor

adaptabi commented Oct 31, 2012

Bummer, this bug still exists in 1.2 Any news when it will get fixed??

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Oct 31, 2012

Member

@klemensz @dotnetwise - Can you provide a test page that shows this is still an issue?

Member

jaspermdegroot commented Oct 31, 2012

@klemensz @dotnetwise - Can you provide a test page that shows this is still an issue?

@adaptabi

This comment has been minimized.

Show comment
Hide comment
@adaptabi

adaptabi Oct 31, 2012

Contributor

Sure, here it is!
http://jsfiddle.net/jzGv5/21/

The same doesn't work for dynamic swap of radio-buttons controlgroup as well.

Generally speaking this is painly hard and contra-intuitive.
Documentation on jqm methods, or the lack of it, really sucks!

You never know what method, with what arguments and on which selector to apply!

jqmDocs really needs a new page with all the methods on the same page, with their options, usage and examples.

Contributor

adaptabi commented Oct 31, 2012

Sure, here it is!
http://jsfiddle.net/jzGv5/21/

The same doesn't work for dynamic swap of radio-buttons controlgroup as well.

Generally speaking this is painly hard and contra-intuitive.
Documentation on jqm methods, or the lack of it, really sucks!

You never know what method, with what arguments and on which selector to apply!

jqmDocs really needs a new page with all the methods on the same page, with their options, usage and examples.

@agcolom

This comment has been minimized.

Show comment
Hide comment
@agcolom

agcolom Nov 1, 2012

Member

@dotnetwise Thanks for reporting a problem with the jQuery Mobile docs (although maybe deserved a more suitable expression). We're currently working on the jQuery Mobile API docs which will be similar to the jQuery UI API docs (http://api.jqueryui.com), which I hope will satisfy your request.

In the mean time, please note that we're always happy to improve the documentation and we welcome contributions via Pull Requests on that matter.

Member

agcolom commented Nov 1, 2012

@dotnetwise Thanks for reporting a problem with the jQuery Mobile docs (although maybe deserved a more suitable expression). We're currently working on the jQuery Mobile API docs which will be similar to the jQuery UI API docs (http://api.jqueryui.com), which I hope will satisfy your request.

In the mean time, please note that we're always happy to improve the documentation and we welcome contributions via Pull Requests on that matter.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Nov 1, 2012

Contributor

OK, so, if this wasn't fixed, I'll reopen it, because #5241 will fix it if merged.

Contributor

gabrielschulhof commented Nov 1, 2012

OK, so, if this wasn't fixed, I'll reopen it, because #5241 will fix it if merged.

@adaptabi

This comment has been minimized.

Show comment
Hide comment
@adaptabi

adaptabi Nov 5, 2012

Contributor

Why was this closed? I still find two issues:

  1. There is no docs generated for the new controlgroup widget whatsoever http://jquerymobile.com/test/docs/forms/
  2. There is still badly written:
    _setType is a private function which cannot be called via controlgroup("_setType")

So still, data-type attribute cannot be changed, even though you change the attribute itself or you do

 $element.jqmData("type", "horizontal");

Is there any other way you can change the $.mobile.controlgroup.prototype.options.type at runtime for a specific controlgroup instance which I am not aware of? (No hacks please)


$element.controlgroup("refresh") works as expected, if you hack the _setType method and make it public, but that's really not desired:

$.mobile.controlgroup.prototype.setType = $.mobile.controlgroup.prototype._setType;

and then later you call it

$element.controlgroup('setType', $element.attr("data-type"));
Contributor

adaptabi commented Nov 5, 2012

Why was this closed? I still find two issues:

  1. There is no docs generated for the new controlgroup widget whatsoever http://jquerymobile.com/test/docs/forms/
  2. There is still badly written:
    _setType is a private function which cannot be called via controlgroup("_setType")

So still, data-type attribute cannot be changed, even though you change the attribute itself or you do

 $element.jqmData("type", "horizontal");

Is there any other way you can change the $.mobile.controlgroup.prototype.options.type at runtime for a specific controlgroup instance which I am not aware of? (No hacks please)


$element.controlgroup("refresh") works as expected, if you hack the _setType method and make it public, but that's really not desired:

$.mobile.controlgroup.prototype.setType = $.mobile.controlgroup.prototype._setType;

and then later you call it

$element.controlgroup('setType', $element.attr("data-type"));
@adaptabi

This comment has been minimized.

Show comment
Hide comment
@adaptabi

adaptabi Nov 15, 2012

Contributor

12 days old and still nothing?

Contributor

adaptabi commented Nov 15, 2012

12 days old and still nothing?

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Nov 15, 2012

Contributor

The issue was closed because the problem was fixed.

You can now add a checkbox to a controlgroup, call the controlgroup's refresh() method, and the controlgroup will be rendered correctly.

If you find issues, please file them separately. For example, the first issue you mention above is indeed present. We intend to address the issue before the next release of jQuery Mobile.

The second issue is invalid. If you consult the API documentation for the jQuery UI widget factory's option() method, you will find that, since "type" is an option of the controlgroup widget, you must call $element.controlgroup( "option", "type", newValue ); where newValue is a variable that holds either the string "horizontal" or the string "vertical".

Contributor

gabrielschulhof commented Nov 15, 2012

The issue was closed because the problem was fixed.

You can now add a checkbox to a controlgroup, call the controlgroup's refresh() method, and the controlgroup will be rendered correctly.

If you find issues, please file them separately. For example, the first issue you mention above is indeed present. We intend to address the issue before the next release of jQuery Mobile.

The second issue is invalid. If you consult the API documentation for the jQuery UI widget factory's option() method, you will find that, since "type" is an option of the controlgroup widget, you must call $element.controlgroup( "option", "type", newValue ); where newValue is a variable that holds either the string "horizontal" or the string "vertical".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment