Skip to content
This repository

collapsible-last not reset during a refresh #4645

Closed
drventure opened this Issue July 04, 2012 · 5 comments

3 participants

Jasper de Groot Maurice Gottlieb

I ran into an issue using jqmobile in conjunction with knockout recently. Essentially, I have knockout generating new COLLAPSIBLE elements inside a COLLAPSIBLE-SET via knockout templates.

That part works just find, but I noticed odd behavior with the styling. It seemed almost as if every added collapsible was the last one, based on the way the corners were being styled.

Turns out, it looks like the refresh method never "resets" the flag "collapsible-last" on any elements. The "last" element has it set properly, but if you're dynamically adding collapsibles and calling refresh to update styling, they all end up marked as "last"

Adding the noted line of code below corrects the problem. Essentially, it just makes sure that collapsible-last is false for all collapsibles in the set before setting it true for the real last one.

''' javascript

    // clean up borders
    collapsiblesInSet.each( function() {
        $(this).jqmData( "collapsible-last", false );    //<--------- this is the added line of code
        $( this ).find( $.mobile.collapsible.prototype.options.heading )
            .find( "a" ).first()
            .add( ".ui-btn-inner" )
            .removeClass( "ui-corner-top ui-corner-bottom" );
    });

    collapsiblesInSet.first()
        .find( "a" )
            .first()
            .addClass( o.corners ? "ui-corner-top" : "" )
            .find( ".ui-btn-inner" )
                .addClass( "ui-corner-top" );

......
'''

Maurice Gottlieb

Hi @drventure, thanks for reporting, good catch btw!
I've built a demo at http://jsfiddle.net/MauriceG/8QGU5/show/light/ After adding a collapsible to a set all looks fine. Until one collapsible is expanded.

@uGoMobi would you agree, if the code in collapsibleSet.js _refresh would be changed to
// clean up borders
collapsiblesInSet.each( function() {
$( this ).jqmData( "collapsible-last", false )
.find( $.mobile.collapsible.prototype.options.heading ) ...

@drventure should create a PR for this?
I've tested this change in local repo and it works fine.
Maurice

Jasper de Groot
Owner

@drventure @MauriceG

I would remove the data object instead of setting it to "false".

// clean up borders
collapsiblesInSet.each( function() {
    $( this ).jqmRemoveData( "collapsible-last" )
        .find( $.mobile.collapsible.prototype.options.heading )
        .find( "a" ).first()
        .removeClass( "ui-corner-top ui-corner-bottom" )
        .find( ".ui-btn-inner" )
        .removeClass( "ui-corner-top ui-corner-bottom" );
});

@drventure

I noticed you are new on Github and haven't forked the jQuery Mobile repo (yet).
If you have a fork you can make changes in the code yourself and then create a pull request (PR). You can read about all this on the Github help pages and here https://github.com/jquery/jquery-mobile#pull-requests.
That's only if you like to contribute to the project that way. Just reporting a bug and suggesting a fix like you did now is highly appreciated as well.

I will change the code to fix this issue.

Jasper de Groot uGoMobi closed this issue from a commit July 05, 2012
Jasper de Groot Collapsible set: Changed the refresh function so it first removes dat…
…a object "collapsible-last" for each item before setting it on the last one. Fixes #4645 - collapsible-last not reset during a refresh. [1.1.2.]
127ef52
Jasper de Groot uGoMobi closed this in 127ef52 July 05, 2012

@uGoMobi

Hi, and Thanks for the comments. Yeah, I'm pretty new to git in general (I've mainly worked with TFS, Subversion and mercurial, kind of a windows guy). I started down the road of pull requests and such, but my eyes were starting to glaze over on all of it, so I thought I'd just go ahead and report this issue directly, as it's such a minor thing.

But thanks for that link. Hadn't seen that one, so I'll work through it and see what I can set up.

I agree about the removeclass. That would seem more in keeping with the rest of the library, now that you mention it.

Thanks!

Jasper de Groot
Owner

@drventure

If you're a Windows guy the new "Clone in Windows" tool from Github might be handy. Never tried it myself though.
Just click the fork button when you are here on the jQuery Mobile repo. That should take you to your own forked repo where you can click the "Clone in Windows" button.

BTW - I didn't change your fix to removeClass, but just changed it from setting the data object to false to completely remove it.

Yeah, sorry about that. i saw your response and misread the code you posted.. Right after I sent that, I realized that was a different change to fix something similar, but not quite the same as the -last tag, and that you'd used that jqmRemoveData function to just remove the tag.

Still getting used to JQuery, knockout and all the libraries/dom manipulation stuff. Quite a bit different from my background.

I'll check out that Clone in Windows tool. I've also read that GIT interoperates quite well with mercurial, so I hope to do some playing around there too.

Jasper de Groot uGoMobi referenced this issue from a commit July 05, 2012
Jasper de Groot Collapsible set: Changed the refresh function so it first removes dat…
…a object "collapsible-last" for each item before setting it on the last one. Fixes #4645 - collapsible-last not reset during a refresh. [1.1.2.]
f418d5d
Elliot Smith townxelliot referenced this issue from a commit July 30, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.