r78865 MediaWiki - Code Review archive

+table.collapsible tr {
+    display: block !important;
+}
+table.collapsible tr {
+    display: table-row !important;
+}

One is redundant.

[CSS]
div.collapsible > * {
...

[JS]
if( $x.is('div.collapsible')){
..
$('div.collapsible')

.collapsible should be usable for any element, not just for div's and tables. There's <p>, <pre>, <center>, <blockquote> etc.

I'm not sure what $.effects.fade is suposed to do what jQuery's core fadeIn() and fadeOut() don't. Please document this in the plugin file.

+				return true;
+			})
+			.before('[').after(']');

There is a [ and ] added already a few lines higher with text()

 	/* Skins */
+	
+	'skins.common' => array(

Although a common file will most likely be needed, I think something like this is best written as a jQuery plugin. Which enables adding elements like these dynamically as well (ie. javascript tools which add a table dynamically)

In the init-function could be a call to automatically enable the functionality on all .collapsible-elements that are on the page when the dom is ready.

ie.

$.fn.collapsibleElement = 
   ....


init:
$('.collapsible').collapsibleElement();

+table.collapsible tr {
+    display: block !important;
+}
+table.collapsible tr {
+    display: table-row !important;
+}

Never mind the comment about one being redundant, it does work this way - but perhaps we can come up with a nicer way.

How? This isn't even particularly ugly CSS hackery :D display:table-row is unsupported in IE8; it's going to have to stay like that for quite some time yet...

This change effects other extensions. Seen on Translatewiki:

PHP Fatal error: Call to undefined method NCL::downArrow() in /www/w/extensions/CleanChanges/CleanChanges_body.php on line 286

+ . "<a href='#' title='$expandTitle'>{$this->arrow( 'd', '-', wfMsg( 'rc-enhanced-hide' ) )}</a>"

+			. "<a href='#' title='$closeTitle'>{$this->arrow( 'd', '-', wfMsg( 'rc-enhanced-hide' ) )}</a>"

One of them is wrong.

I don't like that the arrow helper methods were removed. The direct arrow() calls are much less readable without context.

BC needs to be kept and/or the CleanChanges extension updated. This blocks localisation of MediaWiki core.

Restored in r78889.

{| class="wikitable sortable collapsible collapsed"
! Head
! Top
|-
| Con
| Tent
|}

I saved this to a page in my local install. The [Show] link appeared in the <th>Top</th>, but clicking it did nothing. No errors either.

The link should appear as a link (blue) instead of text, for consistency.

This is an interaction with the sortabletables code; I'm not entirely sure what. The click event is never fired for the show/hide toggle, so I suspect the sortable code is intercepting and stopping its propagation.

I don't particularly see why they should be links; the whole point of the collapsible-expander class is that they can now be anything. There's an obvious usage in the collapsible divs case in particular in making the whole title the expand link, so clicking anywhere on it expands the content.

Although I too can't say for sure at this point what the cause is (could be the sortable code), I recommmend first we finish porting legacy to the new stuff first and making sure it works fine with existing articles etc. – before introducing new things like this.

By the time I've committed the ToC handling, I'll have replaced over a hundred lines of legacy code; this is migration to the new system.

The reason the [show]/[hide] doesn't work inside the tablecell is because a few lines in teh Wikibits use "innerHTML =+" which replaces it's content with something added to it, thus removing any binded handlers to it.

And, although I dont know for sure if you mean this one, the toggleToc function has been assigned to me (bug 26324) and I'm already working on it locally. I would appreciate it if you let me finish it.

Fixed in r78893.

Cool, thanks.

The frontend of the toggleToc() is literally twenty lines:

Index: includes/Linker.php
===================================================================
--- includes/Linker.php	(revision 78891)
+++ includes/Linker.php	(working copy)
@@ -1332,11 +1332,10 @@
 	 */
 	function tocList( $toc ) {
 		$title = wfMsgHtml( 'toc' ) ;
-		return
-		   '<table id="toc" class="toc"><tr><td>'
-		 . '<div id="toctitle"><h2>' . $title . "</h2></div>\n"
-		 . $toc
-		 . "</ul>\n</td></tr></table>\n";
+		return  '<table id="toc" class="toc collapsible"><tr>'
+			. '<th id="toctitle">' . $title . "</th></tr>\n"
+			. '<tr><td>' . $toc
+			. "</ul>\n</td></tr></table>\n";
 	}
 
 	/**
Index: resources/skins.common/skins.common.js
===================================================================
--- resources/skins.common/skins.common.js	(revision 78891)
+++ resources/skins.common/skins.common.js	(working copy)
@@ -104,4 +104,19 @@
 }).trigger( 'click', [false] );
 
 
+$(function() {
+	var cookiePos = document.cookie.indexOf( "hidetoc=" );
+	if ( cookiePos > -1 && document.cookie.charAt( cookiePos + 8 ) == 1 ) {
+		$('#toc .collapsible-expander').click();
+	}
+});
 
+$('#toc .collapsible-expander').click( function() {
+	if ( $('#toc').is('.collapsed') ) {
+		document.cookie = "hidetoc=0";
+	} else {
+		document.cookie = "hidetoc=1";
+	}
+});

The cookie stuff is the only part of the javascript left; and that does indeed need some work.

You can use $.cookie for this, grep the resources directory for examples. Keep in mind that if you use the cookie plugin, the module you use it in has to be marked as depending on jquery.cookie in Resources.php

<div class="collapsible collapsed">
Some text here.
{| class="wikitable collapsible"
! Hello
! World
|-
| Content
| Goes
|-
| In
| Here
|}
Some text here.
</div>

In the above test case the text and table should be hidden on load and appear when clicking "[Show]". But the table heading and the text around are visible by default. And the table in side is incorrectly defaulting to a collapsed-state.

All elements inside the collapsed-element are given display:none.

Fixed in r78891; seems that returning true from an event handler overrides e.stopPropagation().

This commit broke changeslists on translatewiki (unpatrolled-flag exclamation mark popping up on page histories and and diff-views where they shouldn't instead of an 'm' for minor) - something with $oldminor = ChangesList::flag( 'minor' );.

Happy-melon mentioned earlier on IRC that it's okay to remove his collapsible implementation.

The skin.common module can be re-created when we need it again. Right now it would be left empty.

-- r78987

That would have been fairly simple to fix (it's clear that the grep on my IDE is totally FUBAR); but I agree that it's simplest from a CR perspective to clear this all out and start again. I'll reimplement the ChangesList changes with your makeCollapsible plugin now that it supports custom toggles.