r105181 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r105180‎ | r105181 | r105182 >
Date:14:39, 5 December 2011
Author:santhosh
Status:reverted (Comments)
Tags:
Comment:
Add webfont extension support for monobook skin.
Modified paths:
  • /trunk/extensions/WebFonts/WebFonts.php (modified) (history)
  • /trunk/extensions/WebFonts/resources/ext.webfonts.monobook.css (added) (history)
  • /trunk/extensions/WebFonts/resources/ext.webfonts.monobook.js (added) (history)

Diff [purge]

Index: trunk/extensions/WebFonts/resources/ext.webfonts.monobook.css
@@ -0,0 +1,9 @@
 2+
 3+div#webfonts-menu {
 4+ float: none;
 5+}
 6+
 7+ul#webfonts-fontsmenu {
 8+ position: relative;
 9+ margin: 0px;
 10+}
Property changes on: trunk/extensions/WebFonts/resources/ext.webfonts.monobook.css
___________________________________________________________________
Added: svn:eol-style
111 + native
Index: trunk/extensions/WebFonts/resources/ext.webfonts.monobook.js
@@ -0,0 +1,22 @@
 2+/**
 3+ * scripts specific for monobook skin of mediawiki.
 4+ */
 5+
 6+( function ( $ ) {
 7+ /**
 8+ * Prepare the menu for the webfonts.
 9+ * @param config The webfont configuration.
 10+ */
 11+ mw.webfonts.buildMenu = function(config) {
 12+ var $menuItemsDiv = mw.webfonts.buildMenuItems( config );
 13+ if( $menuItemsDiv == null ) {
 14+ return;
 15+ }
 16+ var $div = $( '<div>' )
 17+ .attr( 'id', 'webfonts-menu' )
 18+ .addClass( 'portlet' )
 19+ .append( $( '<h5>' ).text( mw.message( 'webfonts-load' ).escaped() ) )
 20+ .append( $menuItemsDiv .find ('ul').addClass('pBody'));
 21+ $div.insertAfter( '#p-search');
 22+ };
 23+} ) ( jQuery );
Property changes on: trunk/extensions/WebFonts/resources/ext.webfonts.monobook.js
___________________________________________________________________
Added: svn:eol-style
124 + native
Index: trunk/extensions/WebFonts/WebFonts.php
@@ -50,9 +50,13 @@
5151
5252 $wgResourceModules['ext.webfonts.core'] = array(
5353 'scripts' => array( 'resources/ext.webfonts.js', 'resources/ext.webfonts.fontlist.js' ),
 54+ 'skinScripts' => array(
 55+ 'monobook' => 'resources/ext.webfonts.monobook.js',
 56+ ),
5457 'styles' => 'resources/ext.webfonts.css',
5558 'skinStyles' => array(
5659 'modern' => 'resources/ext.webfonts.modern.css',
 60+ 'monobook' => 'resources/ext.webfonts.monobook.css',
5761 ),
5862 'localBasePath' => $dir,
5963 'remoteExtPath' => 'WebFonts',

Follow-up revisions

RevisionCommit summaryAuthorDate
r105286Removed the monobook skin specific code added in r105181....santhosh11:19, 6 December 2011

Comments

#Comment by Nikerabbit (talk | contribs)   16:11, 5 December 2011

text( ..escaped() )?

#Comment by Santhosh.thottingal (talk | contribs)   11:31, 6 December 2011

mw.message( 'webfonts-load' ) is not plain string it is message object. mw.message( 'webfonts-load' ).escaped() gives a plain text that you can pass to .text() of jquery. Alternatively .text(mw.msg( 'webfonts-load' )) will also work fine. mw.msg calls plain method of mw.message

https://www.mediawiki.org/wiki/User:Krinkle/Extension_review/WebFonts#HTML_escaping

#Comment by Nikerabbit (talk | contribs)   16:12, 5 December 2011

Could use some more spaces :)

Functionally ok.

#Comment by Amire80 (talk | contribs)   21:22, 5 December 2011

Some thought about the placement is needed. Currently it says $div.insertAfter( '#p-search');. There are several issues with it:

  • Some projects customize the sidebar appearance.
  • Other extensions may fight for this location, the most immediate example being Narayam.
  • The links after the search may be more important than WebFonts.
#Comment by Santhosh.thottingal (talk | contribs)   11:21, 6 December 2011

Reverted this in r105286

Status & tagging log