jenkins-bot has submitted this change and it was merged.
Change subject: jquery.makeCollapsible: Clean up issues caused by wrong nesting
......................................................................
jquery.makeCollapsible: Clean up issues caused by wrong nesting
The way it was done - switching first on action (expand/collapse), then on
elements - caused the logic to be split all over the file.
This caused:
* code duplication (e.g. computing the elements to be acted upon in
the same way for expanding and collapsing, repeated same comments)
* regressions when the logic was changed for one but not for the other
(this was the case e.g. with table expanding/collapsing).
As for the second, I fixed all spotted inconsistencies; as for the
first, I'll let the diffstat speak for itself.
Change-Id: I2d2592b4d00424f0c23c493f6de6c824d0714dfc
---
M resources/jquery/jquery.makeCollapsible.js
1 file changed, 38 insertions(+), 58 deletions(-)
Approvals:
Krinkle: Looks good to me, approved
jenkins-bot: Verified
diff --git a/resources/jquery/jquery.makeCollapsible.js
b/resources/jquery/jquery.makeCollapsible.js
index 67bec23..b369584 100644
--- a/resources/jquery/jquery.makeCollapsible.js
+++ b/resources/jquery/jquery.makeCollapsible.js
@@ -65,53 +65,67 @@
return;
}
- if ( action === 'collapse' ) {
+ // Handle different kinds of elements
- // Collapse the element
- if ( $collapsible.is( 'table' ) ) {
+ if ( $collapsible.is( 'table' ) ) {
+ // Tables
+ $containers = $collapsible.find( '> tbody > tr'
);
+ if ( $defaultToggle ) {
+ // Exclude table row containing
togglelink
+ $containers = $containers.not(
$defaultToggle.closest( 'tr' ) );
+ }
+
+ if ( action === 'collapse' ) {
// Hide all table rows of this table
- // Slide doens't work with tables, but
fade does as of jQuery 1.1.3
+ // Slide doesn't work with tables, but
fade does as of jQuery 1.1.3
//
http://stackoverflow.com/questions/467336#920480
- $containers = $collapsible.find( '>
tbody > tr' );
- if ( $defaultToggle ) {
- // Exclude tablerow containing
togglelink
- $containers = $containers.not(
$defaultToggle.closest( 'tr' ) );
- }
-
if ( options.instantHide ) {
$containers.hide();
} else {
$containers.stop( true, true
).fadeOut();
}
+ } else {
+ $containers.stop( true, true ).fadeIn();
+ }
- } else if ( $collapsible.is( 'ul' ) ||
$collapsible.is( 'ol' ) ) {
- $containers = $collapsible.find( '> li'
);
- if ( $defaultToggle ) {
- // Exclude list-item containing
togglelink
- $containers = $containers.not(
$defaultToggle.parent() );
- }
+ } else if ( $collapsible.is( 'ul' ) || $collapsible.is(
'ol' ) ) {
+ // Lists
+ $containers = $collapsible.find( '> li' );
+ if ( $defaultToggle ) {
+ // Exclude list-item containing
togglelink
+ $containers = $containers.not(
$defaultToggle.parent() );
+ }
+ if ( action === 'collapse' ) {
if ( options.instantHide ) {
$containers.hide();
} else {
$containers.stop( true, true
).slideUp();
}
-
} else {
- // <div>, <p> etc.
- $collapsibleContent =
$collapsible.find( '> .mw-collapsible-content' );
+ $containers.stop( true, true
).slideDown();
+ }
- // If a collapsible-content is defined,
collapse it
- if ( $collapsibleContent.length ) {
+ } else {
+ // Everything else: <div>, <p> etc.
+ $collapsibleContent = $collapsible.find( '>
.mw-collapsible-content' );
+
+ // If a collapsible-content is defined, act on
it
+ if ( $collapsibleContent.length ) {
+ if ( action === 'collapse' ) {
if ( options.instantHide ) {
$collapsibleContent.hide();
} else {
$collapsibleContent.slideUp();
}
-
- // Otherwise assume this is a
customcollapse with a remote toggle
- // .. and there is no
collapsible-content because the entire element should be toggled
} else {
+ $collapsibleContent.slideDown();
+ }
+
+ // Otherwise assume this is a customcollapse
with a remote toggle
+ // .. and there is no collapsible-content
because the entire element should be toggled
+ } else {
+ if ( action === 'collapse' ) {
if ( options.instantHide ) {
$collapsible.hide();
} else {
@@ -121,40 +135,6 @@
$collapsible.slideUp();
}
}
- }
- }
-
- } else {
-
- // Expand the element
- if ( $collapsible.is( 'table' ) ) {
- $containers = $collapsible.find(
'>tbody>tr' );
- if ( $defaultToggle ) {
- // Exclude tablerow containing
togglelink
- $containers.not(
$defaultToggle.parent().parent() ).stop(true, true).fadeIn();
- } else {
- $containers.stop( true, true
).fadeIn();
- }
-
- } else if ( $collapsible.is( 'ul' ) ||
$collapsible.is( 'ol' ) ) {
- $containers = $collapsible.find( '> li'
);
- if ( $defaultToggle ) {
- // Exclude list-item containing
togglelink
- $containers.not(
$defaultToggle.parent() ).stop( true, true ).slideDown();
- } else {
- $containers.stop( true, true
).slideDown();
- }
-
- } else {
- // <div>, <p> etc.
- $collapsibleContent =
$collapsible.find( '> .mw-collapsible-content' );
-
- // If a collapsible-content is defined,
collapse it
- if ( $collapsibleContent.length ) {
- $collapsibleContent.slideDown();
-
- // Otherwise assume this is a
customcollapse with a remote toggle
- // .. and there is no
collapsible-content because the entire element should be toggled
} else {
if ( $collapsible.is( 'tr' ) ||
$collapsible.is( 'td' ) || $collapsible.is( 'th' ) ) {
$collapsible.fadeIn();
--
To view, visit https://gerrit.wikimedia.org/r/53048
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2d2592b4d00424f0c23c493f6de6c824d0714dfc
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Matmarex <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Matmarex <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
