r102160 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r102159‎ | r102160 | r102161 >
Date:08:02, 6 November 2011
Author:maxsem
Status:ok (Comments)
Tags:miscextensions, todo 
Comment:
Merging ReferenceTooltips into Cite step 2: rename/register
Modified paths:
  • /trunk/extensions/Cite/Cite.php (modified) (history)
  • /trunk/extensions/Cite/modules/ext.cite (added) (history)
  • /trunk/extensions/Cite/modules/ext.cite/ext.cite.js (added) (history)
  • /trunk/extensions/Cite/modules/ext.cite/ext.reference-tooltips.js (deleted) (history)
  • /trunk/extensions/Cite/modules/ext.reference-tooltips (deleted) (history)

Diff [purge]

Index: trunk/extensions/Cite/modules/ext.cite/ext.cite.js
@@ -0,0 +1,12 @@
 2+( function($) {
 3+ $( function() {
 4+ $j('.biblio-cite-link,sup.reference a').tooltip({
 5+ bodyHandler: function() {
 6+ return $j( document.getElementById( this.hash.substr(1) ) )
 7+ .html();
 8+ },
 9+ showURL : false
 10+ } );
 11+ } );
 12+
 13+} )(jQuery);
Property changes on: trunk/extensions/Cite/modules/ext.cite/ext.cite.js
___________________________________________________________________
Added: svn:eol-style
114 + native
Index: trunk/extensions/Cite/Cite.php
@@ -18,7 +18,9 @@
1919 */
2020
2121 $wgHooks['ParserFirstCallInit'][] = 'wfCite';
 22+$wgHooks['BeforePageDisplay'][] = 'wfCiteBeforePageDisplay';
2223
 24+
2325 $wgExtensionCredits['parserhook'][] = array(
2426 'path' => __FILE__,
2527 'name' => 'Cite',
@@ -57,4 +59,34 @@
5860 return Cite::setHooks( $parser );
5961 }
6062
 63+// Resources
 64+$citeResourceTemplate = array(
 65+ 'localBasePath' => dirname(__FILE__) . '/modules',
 66+ 'remoteExtPath' => 'Cite/modules'
 67+);
 68+
 69+$wgResourceModules['ext.cite'] = $citeResourceTemplate + array(
 70+ 'styles' => array(),
 71+ 'scripts' => 'ext.cite/ext.cite.js',
 72+ 'position' => 'bottom',
 73+ 'dependencies' => array(
 74+ 'jquery.tooltip',
 75+ ),
 76+);
 77+
 78+$wgResourceModules['jquery.tooltip'] = $citeResourceTemplate + array(
 79+ 'styles' => 'jquery.tooltip/jquery.tooltip.css',
 80+ 'scripts' => 'jquery.tooltip/jquery.tooltip.js',
 81+ 'position' => 'bottom',
 82+);
 83+
 84+function wfCiteBeforePageDisplay() {
 85+ global $wgOut;
 86+
 87+ $wgOut->addModules( 'ext.cite' );
 88+
 89+ return true;
 90+}
 91+
 92+
6193 /**#@-*/

Follow-up revisions

RevisionCommit summaryAuthorDate
r102173Followup r102160...reedy12:25, 6 November 2011
r106759Deleting ReferenceTooltips extension (r100797) since it was merged to trunk i...demon04:49, 20 December 2011
r108031Follow-up r102160: removed arrows from tooltipsmaxsem15:00, 4 January 2012
r110823Follow-up r102160: disable popups by defaultmaxsem07:04, 7 February 2012

Comments

#Comment by Reedy (talk | contribs)   12:21, 6 November 2011
+function wfCiteBeforePageDisplay() {
+	global $wgOut;
+	
+	$wgOut->addModules( 'ext.cite' );
+	
+	return true;
+}

Why not:

+function wfCiteBeforePageDisplay( $out, &$sk ) {
+	$out->addModules( 'ext.cite' );
+	
+	return true;
+}

? :/

#Comment by Reedy (talk | contribs)   12:24, 6 November 2011

Oh, bad werdna!!!

#Comment by Werdna (talk | contribs)   14:16, 6 November 2011

Blame MaxSem! I didn't do it.

#Comment by MaxSem (talk | contribs)   14:23, 6 November 2011

O RLY? I just copied your code:P

#Comment by Werdna (talk | contribs)   14:24, 6 November 2011

Heh, you win this round.

#Comment by (talk | contribs)   20:27, 9 November 2011

You might should have a look at de:Benutzer:Schnark/js/popuprefs.js. Features: You can click on the Tooltip when pressing shift, for example to select text or follw links; And it really removes the go-up-arrow (or whatever it is in the different refs). And: It has no transparent background.

#Comment by Tim Starling (talk | contribs)   05:36, 25 November 2011

The tooltip gets the wrong styling, because it's not a descendant of #content or #bodyContent, so it misses out on stylesheet rules like:

div#content {
	line-height: 1.5em;
}
#bodyContent {
	font-size: 0.8em;
}
#Comment by Hashar (talk | contribs)   12:14, 5 January 2012

We could either:

  • hack the upstream jquery.tooltip javascript to append the tooltip to $("#bodyContent")
  • Wait for Jquery UI 1.9 which has a clean tooltip: http://wiki.jqueryui.com/Tooltip
#Comment by TheDJ (talk | contribs)   09:12, 25 November 2011

Anyone know if there will be compatibility issues with popups navigator ?

#Comment by Hashar (talk | contribs)   12:15, 5 January 2012

> Anyone know if there will be compatibility issues with popups navigator ?

What is the popups navigator? The Cite popup only trigger on the links matching ".biblio-cite-link,sup.reference a"

#Comment by Schnark (talk | contribs)   10:17, 7 January 2012
#Comment by TheDJ (talk | contribs)   17:34, 16 January 2012

Yeah Navigation popups also triggers on those :D

So likely it wasn't tested in that combination.

#Comment by TheDJ (talk | contribs)   18:23, 24 January 2012

Tested Navigation Popups icw Cite popup: https://commons.wikimedia.org/wiki/File:Screen_shot_2012-01-24_at_10.15.27.png Navigation Popups has been informed.

#Comment by Schnark (talk | contribs)   10:25, 23 December 2011
  • The backlinks in the tooltip are worthless and shouldn't be shown. Just wrap them in some CSS class and remove them in the tooltip.
  • For scripts like the mentioned popups or my script it would be really nice if this could be disabled via javascript.
  • Nice to have: A possibility to click on links in the tooltip.
#Comment by Schnark (talk | contribs)   08:37, 23 January 2012

The tooltip doesn't show up for named refs with special characters, instead an error is thrown. Try <ref name="Ä">x</ref>.

#Comment by TheDJ (talk | contribs)   18:33, 24 January 2012

Can you link an example ? Much easier if anyone wants to follow up on this.

#Comment by TheDJ (talk | contribs)   23:11, 6 February 2012

seems like a jquery bug in it's selectors ?

> document.getElementById('cite_note-.C3.84-0')

  • ​…​
  • ​ > $("#cite_note-.C3.84-0") []

    #Comment by TheDJ (talk | contribs)   23:12, 6 February 2012

    grrr.

    > document.getElementById('cite_note-.C3.84-0')
    <li id=​"cite_note-.C3.84-0">​…​</li>​
    > $("#cite_note-.C3.84-0")
    []
    
    #Comment by Schnark (talk | contribs)   11:02, 7 February 2012

    It's not a bug, it's a feature: $("#cite_note-.C3.84-0") gets all elements with id cite_note- and class C3 and 84-0

    #Comment by Tim Starling (talk | contribs)   22:43, 6 February 2012

    I suggest disabling this by default until the issues above are fixed. If the module is still registered then it can be used by user scripts etc.

    #Comment by MaxSem (talk | contribs)   07:05, 7 February 2012

    Done in r110823, resetting this rev to new.

    Status & tagging log