Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Styling of added checkbox incorrect when in controlgroup #3496

Closed
offsky opened this issue Jan 26, 2012 · 19 comments · Fixed by #4299
Closed

Styling of added checkbox incorrect when in controlgroup #3496

offsky opened this issue Jan 26, 2012 · 19 comments · Fixed by #4299

Comments

@offsky
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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

@toddparker
Copy link
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

@tbosch
Copy link

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
Copy link
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.

@jaspermdegroot
Copy link
Contributor

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
Copy link
Contributor

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

gseguin pushed a commit to gseguin/jquery-mobile that referenced this issue Jun 27, 2012
Fixes jquery-archive#3496 - Styling of added checkbox incorrect when in controlgroup
@klemensz
Copy link

klemensz commented Aug 2, 2012

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

@deakjahn
Copy link

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.

@dotnetwise
Copy link
Contributor

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

@jaspermdegroot
Copy link
Contributor

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

@dotnetwise
Copy link
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.

@agcolom
Copy link
Contributor

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
Copy link

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

@dotnetwise
Copy link
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"));

@dotnetwise
Copy link
Contributor

12 days old and still nothing?

@gabrielschulhof
Copy link

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.