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

Collapsibleset: Make accordion behaviour conditional upon an option #5911

Closed
DzenisevichK opened this Issue Apr 19, 2013 · 14 comments

Comments

Projects
None yet
4 participants
@DzenisevichK

DzenisevichK commented Apr 19, 2013

Currently collapsible-set collapses all siblings related to expanded collapsible that take too much time specially on large collapsible-set.

Why don't specify additional filter (:not(.ui-collapsible-collapsed)) for such collapsibles?

closestCollapsible
                            .siblings( ".ui-collapsible:not(.ui-collapsible-collapsed)" )
                            .trigger( "collapse" );

from collapsibleSet.js

Plus as I understand collapsible-set works always in accordion mode. May be better to have here additional accordion option which by default is true? Or collapsible-set was designed only for accordion functional? I think that not...

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Apr 19, 2013

Member

Thank you for the suggestion however :not is one of the slowest selectors class selectors are much faster. The way :not works internally is it still selects the entire .ui-collapsible element set but then runs the :not selector on this set. since :not is a very slow selector as it has to test every element in the set this would not give any actual performance improvement.

as far as the additional option fell free to add this to the feature request wiki and we will open a ticket just for this if we decide to implement this feature.

Member

arschmitz commented Apr 19, 2013

Thank you for the suggestion however :not is one of the slowest selectors class selectors are much faster. The way :not works internally is it still selects the entire .ui-collapsible element set but then runs the :not selector on this set. since :not is a very slow selector as it has to test every element in the set this would not give any actual performance improvement.

as far as the additional option fell free to add this to the feature request wiki and we will open a ticket just for this if we decide to implement this feature.

@arschmitz arschmitz closed this Apr 19, 2013

@DzenisevichK

This comment has been minimized.

Show comment
Hide comment
@DzenisevichK

DzenisevichK Apr 22, 2013

@arschmitz

How you even may compare performance of :not(.ui-collapsible-collapsed) selector (even it's slowest) with

 .trigger( "collapse" );
collapsible
    .bind( "expand collapse", function( event ) {
        if ( !event.isDefaultPrevented() ) {
            var $this = $( this ),
                isCollapse = ( event.type === "collapse" );

            event.preventDefault();

            collapsibleHeading
                .toggleClass( "ui-collapsible-heading-collapsed", isCollapse )
                .find( ".ui-collapsible-heading-status" )
                    .text( isCollapse ? o.expandCueText : o.collapseCueText )
                .end()
                .find( ".ui-icon" )
                    .toggleClass( "ui-icon-" + o.expandedIcon, !isCollapse )
                    // logic or cause same icon for expanded/collapsed state would remove the ui-icon-class
                    .toggleClass( "ui-icon-" + o.collapsedIcon, ( isCollapse || o.expandedIcon === o.collapsedIcon ) )
                .end()
                .find( "a" ).first().removeClass( $.mobile.activeBtnClass );

            $this.toggleClass( "ui-collapsible-collapsed", isCollapse );
            collapsibleContent.toggleClass( "ui-collapsible-content-collapsed", isCollapse ).attr( "aria-hidden", isCollapse );

            collapsibleContent.trigger( "updatelayout" );
        }
    })

Filtering based on selector will be >100 times faster and this is visible even in a Desktop browser - try with 100 collapsibles set.

DzenisevichK commented Apr 22, 2013

@arschmitz

How you even may compare performance of :not(.ui-collapsible-collapsed) selector (even it's slowest) with

 .trigger( "collapse" );
collapsible
    .bind( "expand collapse", function( event ) {
        if ( !event.isDefaultPrevented() ) {
            var $this = $( this ),
                isCollapse = ( event.type === "collapse" );

            event.preventDefault();

            collapsibleHeading
                .toggleClass( "ui-collapsible-heading-collapsed", isCollapse )
                .find( ".ui-collapsible-heading-status" )
                    .text( isCollapse ? o.expandCueText : o.collapseCueText )
                .end()
                .find( ".ui-icon" )
                    .toggleClass( "ui-icon-" + o.expandedIcon, !isCollapse )
                    // logic or cause same icon for expanded/collapsed state would remove the ui-icon-class
                    .toggleClass( "ui-icon-" + o.collapsedIcon, ( isCollapse || o.expandedIcon === o.collapsedIcon ) )
                .end()
                .find( "a" ).first().removeClass( $.mobile.activeBtnClass );

            $this.toggleClass( "ui-collapsible-collapsed", isCollapse );
            collapsibleContent.toggleClass( "ui-collapsible-content-collapsed", isCollapse ).attr( "aria-hidden", isCollapse );

            collapsibleContent.trigger( "updatelayout" );
        }
    })

Filtering based on selector will be >100 times faster and this is visible even in a Desktop browser - try with 100 collapsibles set.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Apr 22, 2013

Contributor

I believe @DzenisevichK is right. While :not may be the slowest among selectors, even the slowest selector is a lot faster than a single invocation of the signal handling mechanism. So, if we avoid needlessly triggering the "collapse" signal on already-collapsed collapsibles, we still gain a significant advantage.

I will reopen this ticket as a feature request for the option mentioned.

Contributor

gabrielschulhof commented Apr 22, 2013

I believe @DzenisevichK is right. While :not may be the slowest among selectors, even the slowest selector is a lot faster than a single invocation of the signal handling mechanism. So, if we avoid needlessly triggering the "collapse" signal on already-collapsed collapsibles, we still gain a significant advantage.

I will reopen this ticket as a feature request for the option mentioned.

@DzenisevichK

This comment has been minimized.

Show comment
Hide comment
@DzenisevichK

DzenisevichK Apr 23, 2013

@gabrielschulhof

Thanks!
What about to create in jsperf

  • Test 3 with siblings( ".ui-collapsible" ).not( ".ui-collapsible-collapsed" ) and
  • Test 4 with siblings( ".ui-collapsible-expanded" ) (.ui-collapsible-expanded - toggling it as collapsed in $.mobile.collapsible widget, will be good if this new class also reduce css definition too)

DzenisevichK commented Apr 23, 2013

@gabrielschulhof

Thanks!
What about to create in jsperf

  • Test 3 with siblings( ".ui-collapsible" ).not( ".ui-collapsible-collapsed" ) and
  • Test 4 with siblings( ".ui-collapsible-expanded" ) (.ui-collapsible-expanded - toggling it as collapsed in $.mobile.collapsible widget, will be good if this new class also reduce css definition too)
@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Apr 23, 2013

Contributor

Test 3 might be worth performing, but I don't want to introduce a new class just for selection purposes.

Contributor

gabrielschulhof commented Apr 23, 2013

Test 3 might be worth performing, but I don't want to introduce a new class just for selection purposes.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Apr 23, 2013

Contributor

I should also remove test 1, because it would completely skew the results.

Contributor

gabrielschulhof commented Apr 23, 2013

I should also remove test 1, because it would completely skew the results.

@DzenisevichK

This comment has been minimized.

Show comment
Hide comment
@DzenisevichK

DzenisevichK Apr 23, 2013

@gabrielschulhof

Test 3 might be worth performing, but I don't want to introduce a new class just for selection purposes.

Yes, and it seems that Test 4 may be also slower because even 3 toogleClass is necessary (for collapsible, header and content) that will take time...

DzenisevichK commented Apr 23, 2013

@gabrielschulhof

Test 3 might be worth performing, but I don't want to introduce a new class just for selection purposes.

Yes, and it seems that Test 4 may be also slower because even 3 toogleClass is necessary (for collapsible, header and content) that will take time...

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Apr 23, 2013

Contributor

Actually, chaining with .not() is slightly slower: http://jsperf.com/5911-collapsibleset/3

Contributor

gabrielschulhof commented Apr 23, 2013

Actually, chaining with .not() is slightly slower: http://jsperf.com/5911-collapsibleset/3

@DzenisevichK

This comment has been minimized.

Show comment
Hide comment
@DzenisevichK

DzenisevichK Apr 23, 2013

@gabrielschulhof

I have several topic related to performance too. Will try describe in few words:

On our side we work with large data and most critical topic is generating collapsibleset and listview.
Optimization that we use here (for example, in collapsibleset):

  • Clone collapsible widget (without re-creating and refreshing) and change only necessary data (all this done with knockoutjs bindings),
  • Implement collapsible events handling in collapsibleset (delegated handling).

With this we have good performance and only one problem exists - always big deal to update jQueryMobile to latest version.

May be you have ideas how implement delegated handling for collapsibleset and listview in jQueryMobile.

If this topic interesting I may spend time to prepare real example for this.

DzenisevichK commented Apr 23, 2013

@gabrielschulhof

I have several topic related to performance too. Will try describe in few words:

On our side we work with large data and most critical topic is generating collapsibleset and listview.
Optimization that we use here (for example, in collapsibleset):

  • Clone collapsible widget (without re-creating and refreshing) and change only necessary data (all this done with knockoutjs bindings),
  • Implement collapsible events handling in collapsibleset (delegated handling).

With this we have good performance and only one problem exists - always big deal to update jQueryMobile to latest version.

May be you have ideas how implement delegated handling for collapsibleset and listview in jQueryMobile.

If this topic interesting I may spend time to prepare real example for this.

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Aug 6, 2013

Member

We now only collapse collapsibles in a set if they are not collapsed already. I don't see a need to make the accordion behaviour optional because you can use multiple individual collapsibles if you don't want it.

Member

jaspermdegroot commented Aug 6, 2013

We now only collapse collapsibles in a set if they are not collapsed already. I don't see a need to make the accordion behaviour optional because you can use multiple individual collapsibles if you don't want it.

@DzenisevichK

This comment has been minimized.

Show comment
Hide comment
@DzenisevichK

DzenisevichK Aug 8, 2013

@uGoMobi

What the role of collapsiblesset? It's only for supporting accordion?
If not then accordion should be configured with option.

DzenisevichK commented Aug 8, 2013

@uGoMobi

What the role of collapsiblesset? It's only for supporting accordion?
If not then accordion should be configured with option.

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Aug 8, 2013

Member

@DzenisevichK - There is no other reason for using collapsibleSet than to get accordion behaviour.

Member

jaspermdegroot commented Aug 8, 2013

@DzenisevichK - There is no other reason for using collapsibleSet than to get accordion behaviour.

@DzenisevichK

This comment has been minimized.

Show comment
Hide comment
@DzenisevichK

DzenisevichK Aug 8, 2013

@uGoMobi

Now - may be but previously it was possible to inherit collapsibleSet theme by collapsibles (not necessary to define theme per collapsible)...

DzenisevichK commented Aug 8, 2013

@uGoMobi

Now - may be but previously it was possible to inherit collapsibleSet theme by collapsibles (not necessary to define theme per collapsible)...

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Aug 8, 2013

Member

@DzenisevichK

That is still the case, but using a widget just to set options of its children to the same value is not a good idea from a performance point of view.
Every option we add affects the performance of a widget. We actually want to reduce options in widgets and only add them if we don't see any other solution.
As from 1.4 we use CSS for theme inheritance so you could wrap your individual collapsibles in a div with class ui-group-theme-x and they will all inherit theme "x".

Member

jaspermdegroot commented Aug 8, 2013

@DzenisevichK

That is still the case, but using a widget just to set options of its children to the same value is not a good idea from a performance point of view.
Every option we add affects the performance of a widget. We actually want to reduce options in widgets and only add them if we don't see any other solution.
As from 1.4 we use CSS for theme inheritance so you could wrap your individual collapsibles in a div with class ui-group-theme-x and they will all inherit theme "x".

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