r72953 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r72952‎ | r72953 | r72954 >
Date:01:51, 14 September 2010
Author:kaldari
Status:ok (Comments)
Tags:
Comment:
need to set geoMultiSelector display programmatically
Modified paths:
  • /trunk/extensions/CentralNotice/SpecialCentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/centralnotice.css (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/SpecialCentralNotice.php
——@@ -513,7 +513,7 @@
514514 $htmlOut .= Xml::tags( 'td', array(), Xml::label( wfMsgHtml( 'centralnotice-geotargeted' ), 'geotargeted' ) );
515515 $htmlOut .= Xml::tags( 'td', array(), Xml::check( 'geotargeted', false, wfArrayMerge( $readonly, array( 'value' => 1, 'id' => 'geotargeted' ) ) ) );
516516 $htmlOut .= Xml::closeElement( 'tr' );
517 - $htmlOut .= Xml::openElement( 'tr', array( 'id'=>'geoMultiSelector' ) );
 517+ $htmlOut .= Xml::openElement( 'tr', array( 'id'=>'geoMultiSelector', 'style'=>'display:none;' ) );
518518 $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ), wfMsgHtml( 'centralnotice-countries' ) );
519519 $htmlOut .= Xml::tags( 'td', array(), $this->geoMultiSelector() );
520520 $htmlOut .= Xml::closeElement( 'tr' );
——@@ -832,7 +832,11 @@
833833 $htmlOut .= Xml::tags( 'td', array(), Xml::label( wfMsgHtml( 'centralnotice-geotargeted' ), 'geotargeted' ) );
834834 $htmlOut .= Xml::tags( 'td', array(), Xml::check( 'geotargeted', $isGeotargeted, wfArrayMerge( $readonly, array( 'value' => $row->not_name, 'id' => 'geotargeted' ) ) ) );
835835 $htmlOut .= Xml::closeElement( 'tr' );
836 - $htmlOut .= Xml::openElement( 'tr', array( 'id'=>'geoMultiSelector' ) );
 836+ if ( $isGeotargeted ) {
 837+ $htmlOut .= Xml::openElement( 'tr', array( 'id'=>'geoMultiSelector' ) );
 838+ } else {
 839+ $htmlOut .= Xml::openElement( 'tr', array( 'id'=>'geoMultiSelector', 'style'=>'display:none;' ) );
 840+ }
837841 $htmlOut .= Xml::tags( 'td', array( 'valign' => 'top' ), wfMsgHtml( 'centralnotice-countries' ) );
838842 $htmlOut .= Xml::tags( 'td', array(), $this->geoMultiSelector( $countries ) );
839843 $htmlOut .= Xml::closeElement( 'tr' );
Index: trunk/extensions/CentralNotice/centralnotice.css
——@@ -47,9 +47,6 @@
4848 border-style: solid;
4949 border-width: 1px;
5050 }
51 -#preferences tr#geoMultiSelector {
52 - display: none;
53 -}
5451
5552 /* Vector-specific definitions */
5653 body.skin-vector #preferences fieldset.prefsection {

Comments

#Comment by Catrope (talk | contribs)   18:56, 14 September 2010 
-			$htmlOut .= Xml::openElement( 'tr', array( 'id'=>'geoMultiSelector' ) );
+			$htmlOut .= Xml::openElement( 'tr', array( 'id'=>'geoMultiSelector', 'style'=>'display:none;' ) );

Don't do this. It makes the control unusable if JavaScript is turned off.

#Comment by Kaldari (talk | contribs)   19:52, 14 September 2010

If Javascript is turned off then the functionality of the interface is broken. Perhaps I should just replace the page with an error message in that case.

#Comment by Catrope (talk | contribs)   19:54, 14 September 2010

How broken is broken? I understand previewing notices may not work, but I don't see why you couldn't edit a notice or its metadata without JS.

#Comment by Kaldari (talk | contribs)   22:14, 14 September 2010

See reply below.

#Comment by Kaldari (talk | contribs)   22:14, 14 September 2010

There are several features of the interface that will be broken if JS is off. But I guess you have a point that none of the other pieces are critical to being able to edit a campaign. Actually, looking back over this, it looks like if Javascript is disabled, the behavior would actually degrade in an acceptable way. Creating a campaign is always a 2-step process. Editing the campaign after creating it is always required since you can't assign banners (or end dates) until you edit. So having the ability to assign the geotargeting countries during the creation step is actually just a nicety, not a necessity. If Javascript is off, the user would only see the geotargeted checkbox at creation, and if they check it, they would then get to choose the countries on edit, which makes sense I think. This seems less confusing to me than the alternative - showing the countries list when geotargeted is off - and the functionality is still basically the same.

Status & tagging log