r78944 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r78943‎ | r78944 | r78945 >
Date:01:30, 24 December 2010
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
Fixing TODO from r78915
* Removed old "kr-" prefix for the plugin
* Moved function call from the bottom of the script to mw init function
Modified paths:
  • /trunk/phase3/resources/jquery/jquery.makeCollapsible.css (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.makeCollapsible.js (modified) (history)
  • /trunk/phase3/resources/mediawiki.util/mediawiki.util.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.makeCollapsible.js
@@ -4,14 +4,12 @@
55 * This will enable collapsible-functionality on all passed elements.
66 * Will prevent binding twice to the same element.
77 * Initial state is expanded by default, this can be overriden by adding class
8 - * "kr-collapsed" to the "kr-collapsible" element.
9 - * Elements made collapsible have class "kr-made-collapsible".
10 - * Except for tables and lists, the inner content is wrapped in "kr-collapsible-content".
 8+ * "mw-collapsed" to the "mw-collapsible" element.
 9+ * Elements made collapsible have class "mw-made-collapsible".
 10+ * Except for tables and lists, the inner content is wrapped in "mw-collapsible-content".
1111 *
1212 * @author Krinkle <krinklemail@gmail.com>
1313 *
14 - * @TODO: Remove old "kr-" prefix
15 - *
1614 * Dual license:
1715 * @license CC-BY 3.0 <http://creativecommons.org/licenses/by/3.0>
1816 * @license GPL2 <http://www.gnu.org/licenses/old-licenses/gpl-2.0.html>
@@ -21,18 +19,18 @@
2220
2321 return this.each(function() {
2422
25 - var $that = $(this).addClass( 'kr-collapsible' ), // in case $( '#myAJAXelement' ).makeCollapsible() was called
 23+ var $that = $(this).addClass( 'mw-collapsible' ), // in case $( '#myAJAXelement' ).makeCollapsible() was called
2624 that = this,
2725 collapsetext = $(this).attr( 'data-collapsetext' ),
2826 expandtext = $(this).attr( 'data-expandtext' ),
2927 toggleFunction = function( that ) {
3028 var $that = $(that),
31 - $collapsible = $that.closest( '.kr-collapsible.kr-made-collapsible' ).toggleClass( 'kr-collapsed' );
 29+ $collapsible = $that.closest( '.mw-collapsible.mw-made-collapsible' ).toggleClass( 'mw-collapsed' );
3230
3331 // It's expanded right now
34 - if ( $that.hasClass( 'kr-collapsible-toggle-expanded' ) ) {
 32+ if ( $that.hasClass( 'mw-collapsible-toggle-expanded' ) ) {
3533 // Change link to "Show"
36 - $that.removeClass( 'kr-collapsible-toggle-expanded' ).addClass( 'kr-collapsible-toggle-collapsed' );
 34+ $that.removeClass( 'mw-collapsible-toggle-expanded' ).addClass( 'mw-collapsible-toggle-collapsed' );
3735 if ( $that.find( '> a' ).size() ) {
3836 $that.find( '> a' ).text( expandtext );
3937 } else {
@@ -50,13 +48,13 @@
5149 $collapsible.find( '> li' ).not( $that.parent() ).stop( true, true ).slideUp();
5250
5351 } else { // <div>, <p> etc.
54 - $collapsible.find( '> .kr-collapsible-content' ).slideUp();
 52+ $collapsible.find( '> .mw-collapsible-content' ).slideUp();
5553 }
5654
5755 // It's collapsed right now
5856 } else {
5957 // Change link to "Hide"
60 - $that.removeClass( 'kr-collapsible-toggle-collapsed' ).addClass( 'kr-collapsible-toggle-expanded' );
 58+ $that.removeClass( 'mw-collapsible-toggle-collapsed' ).addClass( 'mw-collapsible-toggle-expanded' );
6159 if ( $that.find( '> a' ).size() ) {
6260 $that.find( '> a' ).text( collapsetext );
6361 } else {
@@ -70,7 +68,7 @@
7169 $collapsible.find( '> li' ).not( $that.parent() ).stop( true, true ).slideDown();
7270
7371 } else { // <div>, <p> etc.
74 - $collapsible.find( '> .kr-collapsible-content' ).slideDown();
 72+ $collapsible.find( '> .mw-collapsible-content' ).slideDown();
7573 }
7674 }
7775 return;
@@ -85,23 +83,23 @@
8684 }
8785
8886 // 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){
 87+ var $toggleLink = $( '<a href="#">' ).text( collapsetext ).wrap( '<span class="mw-collapsible-toggle mw-collapsible-toggle-expanded">' ).parent().prepend( '&nbsp;[' ).append( ']&nbsp;' ).click( function(e){
9088 e.preventDefault();
9189 toggleFunction( this );
9290 } );
9391
9492 // Skip if it has been enabled already.
95 - if ( $that.hasClass( 'kr-made-collapsible' ) ) {
 93+ if ( $that.hasClass( 'mw-made-collapsible' ) ) {
9694 return;
9795 } else {
98 - $that.addClass( 'kr-made-collapsible' );
 96+ $that.addClass( 'mw-made-collapsible' );
9997 }
10098
10199 // Elements are treated differently
102100 if ( $that.is( 'table' ) ) {
103101 // The toggle-link will be in the last cell (td or th) of the first row
104102 var $lastCell = $( 'tr:first th, tr:first td', that ).eq(-1),
105 - $toggle = $lastCell.find( '> .kr-collapsible-toggle' );
 103+ $toggle = $lastCell.find( '> .mw-collapsible-toggle' );
106104
107105 if ( !$toggle.size() ) {
108106 $lastCell.prepend( $toggleLink );
@@ -115,14 +113,14 @@
116114 } else if ( $that.is( 'ul' ) || $that.is( 'ol' ) ) {
117115 // The toggle-link will be in the first list-item
118116 var $firstItem = $( 'li:first', $that),
119 - $toggle = $firstItem.find( '> .kr-collapsible-toggle' );
 117+ $toggle = $firstItem.find( '> .mw-collapsible-toggle' );
120118
121119 if ( !$toggle.size() ) {
122120 // Make sure the numeral order doesn't get messed up, reset to 1 unless value-attribute is already used
123121 if ( $firstItem.attr( 'value' ) == '' ) {
124122 $firstItem.attr( 'value', '1' );
125123 }
126 - $that.prepend( $toggleLink.wrap( '<li class="kr-collapsibile-toggle-li">' ).parent() );
 124+ $that.prepend( $toggleLink.wrap( '<li class="mw-collapsibile-toggle-li">' ).parent() );
127125 } else {
128126 $toggleLink = $toggle.unbind( 'click' ).click( function( e ){
129127 e.preventDefault();
@@ -133,12 +131,12 @@
134132 } else { // <div>, <p> etc.
135133 // Is there an content-wrapper already made ?
136134 // If a direct child with the class does not exists, create the wrap.
137 - if ( !$that.find( '> .kr-collapsible-content' ).size() ) {
138 - $that.wrapInner( '<div class="kr-collapsible-content">' );
 135+ if ( !$that.find( '> .mw-collapsible-content' ).size() ) {
 136+ $that.wrapInner( '<div class="mw-collapsible-content">' );
139137 }
140138
141139 // The toggle-link will be the first child of the element
142 - var $toggle = $that.find( '> .kr-collapsible-toggle' );
 140+ var $toggle = $that.find( '> .mw-collapsible-toggle' );
143141
144142 if ( !$toggle.size() ) {
145143 $that.prepend( $toggleLink );
@@ -152,11 +150,8 @@
153151 }
154152 console.log( $toggleLink.get(0) );
155153 // Initial state
156 - if ( $that.hasClass( 'kr-collapsed' ) ) {
 154+ if ( $that.hasClass( 'mw-collapsed' ) ) {
157155 $toggleLink.click();
158156 }
159157 } );
160 -};
161 -$( function(){
162 - $( '.kr-collapsible' ).makeCollapsible();
163 -} );
\ No newline at end of file
 158+};
\ No newline at end of file
Index: trunk/phase3/resources/jquery/jquery.makeCollapsible.css
@@ -1,20 +1,20 @@
22 /* See also jquery.makeCollapsible.js */
3 -.kr-collapsible-toggle {
 3+.mw-collapsible-toggle {
44 float:right;
55 }
6 -.kr-collapsible-toggle-expanded,
7 -.kr-collapsible-toggle-expanded a {
 6+.mw-collapsible-toggle-expanded,
 7+.mw-collapsible-toggle-expanded a {
88 cursor:n-resize;
99 }
10 -.kr-collapsible-toggle-collapsed,
11 -.kr-collapsible-toggle-collapsed a {
 10+.mw-collapsible-toggle-collapsed,
 11+.mw-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 */
15 -li .kr-collapsible-toggle {
 15+li .mw-collapsible-toggle {
1616 float:none;
1717 }
1818 /* the added list item should have no list-style */
19 -.kr-collapsibile-toggle-li {
 19+.mw-collapsibile-toggle-li {
2020 list-style:none;
2121 }
\ No newline at end of file
Index: trunk/phase3/resources/mediawiki.util/mediawiki.util.js
@@ -65,6 +65,9 @@
6666 } else {
6767 mw.util.$content = $( '#content' );
6868 }
 69+
 70+ /* Enable makeCollapse */
 71+ $( '.mw-collapsible' ).makeCollapsible();
6972
7073 /* Table of Contents toggle */
7174 var $tocContainer = $( '#toc' ),

Follow-up revisions

RevisionCommit summaryAuthorDate
r78965Forgot to namespace the click handler. Unbinding all click handlers is indeed...krinkle15:24, 24 December 2010
r78969Fixing typo from r78944krinkle16:08, 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 Happy-melon (talk | contribs)   14:49, 24 December 2010
$that.prepend( $toggleLink.wrap( '<li class="mw-collapsibile-toggle-li">' ).parent() );
...
+.mw-collapsibile-toggle-li {

Typo?

$toggleLink = $toggle.unbind( 'click' ).click( function( e ){

Why is it necessary to unbind other events? That seems dangerous to me.

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

Typo fixed in r78969.

Unbinding is done to avoid it being bound twice, however just 'click' is indeed too general and could cause other (not directly related) bindings to get lost. I've added a mw-collapsible namespace to the event in r78965.

Status & tagging log