r78935 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r78934‎ | r78935 | r78936 >
Date:23:34, 23 December 2010
Author:krinkle
Status:resolved (Comments)
Tags:
Comment:
Improving jquery.makeCollapsible:
* Setting toggle function in a variable to easier reuse it through the plugin
* Replace "-hide" and "-show" classes with "expanded" and "collapsed" respectively
* For more flexibility, if there's a toggle-element already, bind the function to it and use it instead. Otherwise, just create one as usual. See [[betawiki:User:Krinkle/CollapsingTestpageKr#footer|CollapsingTestpageKr#Combination example]]
** It's more likely that the source has a custom position of the togglelink waiting for it to be bound, then it to be already bound.
* Some comments improved
* Message key "hide" to "collapsible-collapse" (Follow-up r78915 CR)
* Message key "show" to "collapsible-expand"
* Changed toggle-link content filling from html() to text() (message errors like <keyname> caused an HTML-element to be created)
Modified paths:
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.makeCollapsible.css (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.makeCollapsible.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.makeCollapsible.js
——@@ -20,62 +20,78 @@
2121 $.fn.makeCollapsible = function() {
2222
2323 return this.each(function() {
 24+
2425 var $that = $(this).addClass( 'kr-collapsible' ), // in case $( '#myAJAXelement' ).makeCollapsible() was called
2526 that = this,
2627 collapsetext = $(this).attr( 'data-collapsetext' ),
27 - expandtext = $(this).attr( 'data-expandtext' );
 28+ expandtext = $(this).attr( 'data-expandtext' ),
 29+ toggleFunction = function( that ) {
 30+ var $that = $(that),
 31+ $collapsible = $that.closest( '.kr-collapsible.kr-made-collapsible' ).toggleClass( 'kr-collapsed' );
 32+
 33+ // It's expanded right now
 34+ if ( $that.hasClass( 'kr-collapsible-toggle-expanded' ) ) {
 35+ // Change link to "Show"
 36+ $that.removeClass( 'kr-collapsible-toggle-expanded' ).addClass( 'kr-collapsible-toggle-collapsed' );
 37+ if ( $that.find( '> a' ).size() ) {
 38+ $that.find( '> a' ).text( expandtext );
 39+ } else {
 40+ $that.text( expandtext );
 41+ }
 42+ // Hide the collapsible element
 43+ if ( $collapsible.is( 'table' ) ) {
 44+ // Hide all direct childing table rows of this table, except the row containing the link
 45+ // Slide doens't work, but fade works fine as of jQuery 1.1.3
 46+ // http://stackoverflow.com/questions/467336/jquery-how-to-use-slidedown-or-collapsed-function-on-a-table-row#920480
 47+ // Stop to prevent animations from stacking up
 48+ $collapsible.find( '> tbody > tr' ).not( $that.parent().parent() ).stop( true, true ).fadeOut();
 49+
 50+ } else if ( $collapsible.is( 'ul' ) || $collapsible.is( 'ol' ) ) {
 51+ $collapsible.find( '> li' ).not( $that.parent() ).stop( true, true ).slideUp();
 52+
 53+ } else { // <div>, <p> etc.
 54+ $collapsible.find( '> .kr-collapsible-content' ).slideUp();
 55+ }
 56+
 57+ // It's collapsed right now
 58+ } else {
 59+ // Change link to "Hide"
 60+ $that.removeClass( 'kr-collapsible-toggle-collapsed' ).addClass( 'kr-collapsible-toggle-expanded' );
 61+ if ( $that.find( '> a' ).size() ) {
 62+ $that.find( '> a' ).text( collapsetext );
 63+ } else {
 64+ $that.text( collapsetext );
 65+ }
 66+ // Show the collapsible element
 67+ if ( $collapsible.is( 'table' ) ) {
 68+ $collapsible.find( '> tbody > tr' ).not( $that.parent().parent() ).stop( true, true ).fadeIn();
 69+
 70+ } else if ( $collapsible.is( 'ul' ) || $collapsible.is( 'ol' ) ) {
 71+ $collapsible.find( '> li' ).not( $that.parent() ).stop( true, true ).slideDown();
 72+
 73+ } else { // <div>, <p> etc.
 74+ $collapsible.find( '> .kr-collapsible-content' ).slideDown();
 75+ }
 76+ }
 77+ return;
 78+ };
 79+
2880 // Use custom text or default ?
2981 if( !collapsetext || collapsetext == '' ){
30 - collapsetext = mw.msg( 'hide' );
 82+ collapsetext = mw.msg( 'collapsible-collapse' );
3183 }
3284 if ( !expandtext || expandtext == '' ){
33 - expandtext = mw.msg( 'show' );
 85+ expandtext = mw.msg( 'collapsible-expand' );
3486 }
35 - // Create toggle link with a space around the brackets (&nbsp)
36 - $toggleLink = $( '<span class="kr-collapsible-toggle kr-collapsible-toggle-hide">&nbsp;[<a href="#">' + collapsetext + '</a>]&nbsp;</span>' ).click( function(){
37 - var $that = $(this),
38 - $collapsible = $that.closest( '.kr-collapsible.kr-made-collapsible' ).toggleClass( 'kr-collapsed' );
39 - if ( $that.hasClass( 'kr-collapsible-toggle-hide' ) ) {
40 - // Change link to "Show"
41 - $that
42 - .removeClass( 'kr-collapsible-toggle-hide' ).addClass( 'kr-collapsible-toggle-show' )
43 - .find( '> a' ).html( expandtext );
44 - // Hide the collapsible element
45 - if ( $collapsible.is( 'table' ) ) {
46 - // Hide all direct childing table rows of this table, except the row containing the link
47 - // Slide doens't work, but fade works fine as of jQuery 1.1.3
48 - // http://stackoverflow.com/questions/467336/jquery-how-to-use-slidedown-or-show-function-on-a-table-row#920480
49 - // Stop to prevent animaties from stacking up
50 - $collapsible.find( '> tbody > tr' ).not( $that.parent().parent() ).stop(true, true).fadeOut();
5187
52 - } else if ( $collapsible.is( 'ul' ) || $collapsible.is( 'ol' ) ) {
53 - $collapsible.find( '> li' ).not($that.parent()).stop(true, true).slideUp();
54 -
55 - } else { // <div>, <p> etc.
56 - $collapsible.find( '> .kr-collapsible-content' ).slideUp();
57 - }
58 -
59 - } else {
60 - // Change link to "Hide"
61 - $that
62 - .removeClass( 'kr-collapsible-toggle-show' ).addClass( 'kr-collapsible-toggle-hide' )
63 - .find( '> a' ).html( collapsetext );
64 - // Show the collapsible element
65 - if ( $collapsible.is( 'table' ) ) {
66 - $collapsible.find( '> tbody > tr' ).not( $that.parent().parent() ).stop(true, true).fadeIn();
67 -
68 - } else if ( $collapsible.is( 'ul' ) || $collapsible.is( 'ol' ) ) {
69 - $collapsible.find( '> li' ).not( $that.parent() ).stop(true, true).slideDown();
70 -
71 - } else { // <div>, <p> etc.
72 - $collapsible.find( '> .kr-collapsible-content' ).slideDown();
73 - }
74 - }
75 - return false;
 88+ // Create toggle link with a space around the brackets (&nbsp;[text]&nbsp;)
 89+ var $toggleLink = $( '<a href="#">' ).text( collapsetext ).wrap( '<span class="kr-collapsible-toggle kr-collapsible-toggle-expanded">' ).parent().prepend( '&nbsp;[' ).append( ']&nbsp;' ).click( function(e){
 90+ e.preventDefault();
 91+ toggleFunction( this );
7692 } );
7793
7894 // Skip if it has been enabled already.
79 - if ($that.hasClass( 'kr-made-collapsible' )) {
 95+ if ( $that.hasClass( 'kr-made-collapsible' ) ) {
8096 return;
8197 } else {
8298 $that.addClass( 'kr-made-collapsible' );
——@@ -84,19 +100,36 @@
85101 // Elements are treated differently
86102 if ( $that.is( 'table' ) ) {
87103 // The toggle-link will be in the last cell (td or th) of the first row
88 - var $lastCell = $( 'tr:first th, tr:first td', that ).eq(-1);
89 - if ( !$lastCell.find( '> .kr-collapsible-toggle' ).size() ) {
 104+ var $lastCell = $( 'tr:first th, tr:first td', that ).eq(-1),
 105+ $toggle = $lastCell.find( '> .kr-collapsible-toggle' );
 106+
 107+ if ( !$toggle.size() ) {
90108 $lastCell.prepend( $toggleLink );
 109+ } else {
 110+ $toggleLink = $toggle.unbind( 'click' ).click( function( e ){
 111+ e.preventDefault();
 112+ toggleFunction( this );
 113+ } );
91114 }
92115
93 - } else if ($that.is( 'ul' ) || $that.is( 'ol' )) {
94 - if ( !$( 'li:first', $that).find( '> .kr-collapsible-toggle' ).size() ) {
95 - // Make sure the numeral count doesn't get messed up, reset to 1 unless value-attribute is already used
96 - if ( $( 'li:first', $that ).attr( 'value' ) == '' ) {
97 - $( 'li:first', $that ).attr( 'value', '1' );
 116+ } else if ( $that.is( 'ul' ) || $that.is( 'ol' ) ) {
 117+ // The toggle-link will be in the first list-item
 118+ var $firstItem = $( 'li:first', $that),
 119+ $toggle = $firstItem.find( '> .kr-collapsible-toggle' );
 120+
 121+ if ( !$toggle.size() ) {
 122+ // Make sure the numeral order doesn't get messed up, reset to 1 unless value-attribute is already used
 123+ if ( $firstItem.attr( 'value' ) == '' ) {
 124+ $firstItem.attr( 'value', '1' );
98125 }
99126 $that.prepend( $toggleLink.wrap( '<li class="kr-collapsibile-toggle-li">' ).parent() );
100 - }
 127+ } else {
 128+ $toggleLink = $toggle.unbind( 'click' ).click( function( e ){
 129+ e.preventDefault();
 130+ toggleFunction( this );
 131+ } );
 132+ }
 133+
101134 } else { // <div>, <p> etc.
102135 // Is there an content-wrapper already made ?
103136 // If a direct child with the class does not exists, create the wrap.
——@@ -104,11 +137,20 @@
105138 $that.wrapInner( '<div class="kr-collapsible-content">' );
106139 }
107140
108 - // Add toggle-link if not present
109 - if ( !$that.find( '> .kr-collapsible-toggle' ).size() ) {
 141+ // The toggle-link will be the first child of the element
 142+ var $toggle = $that.find( '> .kr-collapsible-toggle' );
 143+
 144+ if ( !$toggle.size() ) {
110145 $that.prepend( $toggleLink );
 146+ } else {
 147+ $toggleLink = $toggle.unbind( 'click' ).click( function( e ){
 148+ e.preventDefault();
 149+ toggleFunction( this );
 150+ } );
111151 }
 152+
112153 }
 154+ console.log( $toggleLink.get(0) );
113155 // Initial state
114156 if ( $that.hasClass( 'kr-collapsed' ) ) {
115157 $toggleLink.click();
Index: trunk/phase3/resources/jquery/jquery.makeCollapsible.css
——@@ -2,12 +2,12 @@
33 .kr-collapsible-toggle {
44 float:right;
55 }
6 -.kr-collapsible-toggle-hide,
7 -.kr-collapsible-toggle-hide a {
 6+.kr-collapsible-toggle-expanded,
 7+.kr-collapsible-toggle-expanded a {
88 cursor:n-resize;
99 }
10 -.kr-collapsible-toggle-show,
11 -.kr-collapsible-toggle-show a {
 10+.kr-collapsible-toggle-collapsed,
 11+.kr-collapsible-toggle-collapsed a {
1212 cursor:s-resize;
1313 }
1414 /* list-items go as wide as their parent element, don't float them inside list items */
Index: trunk/phase3/resources/Resources.php
——@@ -81,6 +81,7 @@
8282 'jquery.makeCollapsible' => array(
8383 'scripts' => 'resources/jquery/jquery.makeCollapsible.js',
8484 'styles' => 'resources/jquery/jquery.makeCollapsible.css',
 85+ 'messages' => array( 'collapsible-expand', 'collapsible-collapse' ),
8586 ),
8687 'jquery.suggestions' => array(
8788 'scripts' => 'resources/jquery/jquery.suggestions.js',

Follow-up revisions

RevisionCommit summaryAuthorDate
r78961Removing debug call. Follow-up r78935 CRkrinkle14:34, 24 December 2010
r78965Forgot to namespace the click handler. Unbinding all click handlers is indeed...krinkle15:24, 24 December 2010
r78967Adding message for collapsible link in core for localization (Follow-up r7893...krinkle15:52, 24 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r78915whitespace according to mw conventions + using mw.msg instead of english hard...krinkle19:26, 23 December 2010

Comments

#Comment by Krinkle (talk | contribs)   23:39, 23 December 2010

Commit message didn't parse the link:

* ..
* For more flexibility, if there's a toggle-element already, bind the function to it and use it instead. Otherwise, just create one as usual. See CollapsingTestpageKr#Combination example
* ..
#Comment by Happy-melon (talk | contribs)   14:21, 24 December 2010

+ console.log( $toggleLink.get(0) ); Debug? If not, use mw.log.

#Comment by Krinkle (talk | contribs)   14:34, 24 December 2010

Thanks, fixed in r78961

#Comment by Happy-melon (talk | contribs)   14:50, 24 December 2010

Specifying the link text as custom attributes breaks XHTML compliance, which will make bot owners hate you (http://validator.w3.org/check?uri=http%3A%2F%2Ftranslatewiki.net%2Fwiki%2FUser%3AKrinkle%2FCollapsingTestpageKr&charset=%28detect+automatically%29&doctype=XHTML+1.0+Transitional&group=1&st=1&user-agent=W3C_Validator%2F1.1, look for "there is no attribute X"); and is also bad for i18n. Admittedly I can't think of a way of doing it which isn't bad for i18n, but there should certainly be a nicer way of doing it than custom attributes.

I think the JS code of this implementation is cleaner, although more verbose, than my r78865 and friends. However, I don't see the ability to specify custom collapse toggles which you claim was added, which is necessary for the RC/watchlist feed. Am I missing something?

#Comment by Krinkle (talk | contribs)   15:16, 24 December 2010

That validator URL forces XHTML 1.0 Transitional on the page, but TranslateWiki is in HTML5. HTML5 Validation doesn't give errors for those attributes since data-* attributes are valid in HTML5.

On WMF HTML5 is turned off, and with that MediaWiki strips those attributes so the labels degrade to the default localized messages for "collapsible-collapse" and "collapsible-expand."

#Comment by Happy-melon (talk | contribs)   15:17, 24 December 2010

You haven't actually added the "collapsible-(expand|collapse)" messages.

#Comment by Krinkle (talk | contribs)   15:53, 24 December 2010

messages uses since r78915, changed to this key in r78935. Added to translatewiki, added to core in r78967

Status & tagging log