r93353 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r93352‎ | r93353 | r93354 >
Date:00:48, 28 July 2011
Author:kaldari
Status:ok (Comments)
Tags:
Comment:
new banner hiding code, can hide fundraising banners separately
Modified paths:
  • /trunk/extensions/CentralNotice/centralnotice.js (modified) (history)
  • /trunk/extensions/CentralNotice/special/SpecialBannerController.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/special/SpecialBannerController.php
@@ -54,7 +54,7 @@
5555 'getVars': {}
5656 },
5757 'fn': {
58 - 'loadBanner': function( bannerName, campaign ) {
 58+ 'loadBanner': function( bannerName, campaign, bannerType ) {
5959 // Get the requested banner
6060 var bannerPageQuery = $.param( {
6161 'banner': bannerName, 'campaign': campaign, 'userlang': wgUserLanguage,
@@ -66,9 +66,11 @@
6767 Xml::escapeJsString( $wgCentralPagePath ) .
6868 "' + bannerPage + '\"></script>';\n";
6969 $js .= <<<JAVASCRIPT
70 - $( '#siteNotice' ).prepend( '<div id="centralNotice" class="' +
71 - ( wgNoticeToggleState ? 'expanded' : 'collapsed' ) +
72 - '">'+bannerScript+'</div>' );
 70+ if ( document.cookie.indexOf( 'centralnotice_'+bannerType+'=hide' ) == -1 ) {
 71+ $( '#siteNotice' ).prepend( '<div id="centralNotice" class="' +
 72+ ( wgNoticeToggleState ? 'expanded' : 'collapsed' ) +
 73+ ' cn-' + bannerType + '">'+bannerScript+'</div>' );
 74+ }
7375 },
7476 'loadBannerList': function( geoOverride ) {
7577 if ( geoOverride ) {
@@ -118,7 +120,8 @@
119121 // Load a random banner from our groomed list
120122 $.centralNotice.fn.loadBanner(
121123 groomedBannerList[pointer].name,
122 - groomedBannerList[pointer].campaign
 124+ groomedBannerList[pointer].campaign,
 125+ ( groomedBannerList[pointer].fundraising ? 'fundraising' : 'default' )
123126 );
124127 },
125128 'getQueryStringVariables': function() {
@@ -177,14 +180,25 @@
178181 }
179182 }
180183 }
 184+function hideBanner( bannerType ) {
 185+ $( '#centralNotice' ).hide(); // Hide current banner
 186+ if ( bannerType === undefined ) bannerType = 'default';
 187+ setBannerHidingCookie( bannerType ); // Hide future banners of the same type
 188+}
 189+function setBannerHidingCookie( bannerType ) {
 190+ var e = new Date();
 191+ e.setTime( e.getTime() + (7*24*60*60*1000) ); // one week
 192+ var work='centralnotice_'+bannerType+'=hide; expires=' + e.toGMTString() + '; path=/';
 193+ document.cookie = work;
 194+}
181195 function toggleNotice() {
182196 var notice = document.getElementById('centralNotice');
183197 if (!wgNoticeToggleState) {
184198 notice.className = notice.className.replace('collapsed', 'expanded');
185 - toggleNoticeCookie('0');
 199+ toggleNoticeCookie('0'); // Expand banners
186200 } else {
187201 notice.className = notice.className.replace('expanded', 'collapsed');
188 - toggleNoticeCookie('1');
 202+ toggleNoticeCookie('1'); // Collapse banners
189203 }
190204 wgNoticeToggleState = !wgNoticeToggleState;
191205 }
Index: trunk/extensions/CentralNotice/centralnotice.js
@@ -45,7 +45,11 @@
4646 var bannerField = document.getElementById('templateBody');
4747 switch( buttonType ) {
4848 case 'close': // Insert close button
49 - var buttonValue = '<a href="#" onclick="toggleNotice();return false"><img border="0" src="'+stylepath+'/common/images/closewindow.png" alt="Close" /></a>';
 49+ if ( $( '#fundraising' ).is( ':checked' ) ) {
 50+ var buttonValue = '<a href="#" onclick="hideBanner(\'fundraising\');return false;"><img border="0" src="'+stylepath+'/common/images/closewindow.png" alt="Close" /></a>';
 51+ } else {
 52+ var buttonValue = '<a href="#" onclick="hideBanner();return false;"><img border="0" src="'+stylepath+'/common/images/closewindow.png" alt="Close" /></a>';
 53+ }
5054 break;
5155 }
5256 if (document.selection) {

Sign-offs

UserFlagDate
Awjrichardsinspected05:17, 28 July 2011

Comments

#Comment by Awjrichards (talk | contribs)   05:18, 28 July 2011

At a glance this looks good to me - and a sensible/smart way to deal with the banner hiding issue. However my lack of confidence in my Javascript skills is preventing me from marking this OK

#Comment by Awjrichards (talk | contribs)   05:30, 28 July 2011

On second thought:

( groomedBannerList[pointer].fundraising ? 'fundraising' : 'default' )

Would this pose a problem for someone who otherwise explicitly set their banner type?

#Comment by Awjrichards (talk | contribs)   18:45, 28 July 2011

From Kaldari: If someone explicitly sets a banner type right now (other than fundraising), it will be ignored and the banner will get dumped into the "default" bucket. This is by design for the time being. After we've tested this new system to make sure it works, it will be simple to switch it to a full "banner-group" system. We'll just replace the fundraising field with a bannerType field, replace the line above with...

 groomedBannerList[pointer].bannerType

add in a simple user interface for selecting or entering the bannerType group, and change a couple lines in the Javascript that inserts the close button. Everything else is ready to go. For the time being, however, it is limited to just 2 groups: 'fundraising' and 'default', which should make it easier to field test.

Status & tagging log