r112201 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r112200‎ | r112201 | r112202 >
Date:13:16, 23 February 2012
Author:jeroendedauw
Status:deferred (Comments)
Tags:
Comment:
slightly modified patch by MWJames from bug 34411
Modified paths:
  • /trunk/extensions/SemanticResultFormats/Gallery/SRF_Gallery.php (modified) (history)
  • /trunk/extensions/SemanticResultFormats/Gallery/resources/ext.srf.jcarousel.css (modified) (history)
  • /trunk/extensions/SemanticResultFormats/Gallery/resources/ext.srf.jcarousel.js (modified) (history)

Diff [purge]

Index: trunk/extensions/SemanticResultFormats/Gallery/SRF_Gallery.php
——@@ -1,4 +1,4 @@
2 -<?php
 2+<?php
33
44 /**
55 * Result printer that prints query results as a gallery.
——@@ -7,7 +7,8 @@
88 * @ingroup SemanticResultFormats
99 *
1010 * @author Rowan Rodrik van der Molen
11 - * @author Jeroen De Dauw, mwjames
 11+ * @author Jeroen De Dauw
 12+ * @author mwjames
1213 */
1314 class SRFGallery extends SMWResultPrinter {
1415
——@@ -29,18 +30,35 @@
3031 $ig->setShowFilename( false );
3132 $ig->setParser( $wgParser );
3233 $ig->setCaption( $this->mIntro ); // set caption to IQ header
33 -
 34+
3435 if ( $this->m_params['galleryformat'] == 'carousel' ) {
3536 // Set attributes for jcarousel
36 - $mAttribs['id'] = 'carousel';
 37+ $mAttribs['id'] = 'carousel';
3738 $mAttribs['class'] = 'jcarousel-skin-smw';
 39+
 40+ // Aoid js loading issues by not displaying anything until js is able to do so
3841 $mAttribs['style'] = 'display:none;';
 42+
 43+ // Horizontal or vertical orientation
 44+ $mAttribs['orientation'] = 'horizontal';
 45+
 46+ // Whether to wrap at the first/last item (or both) and jump back to the start/end
 47+ $mAttribs['wrap'] = 'both';
 48+
 49+ // Use perrow parameter to determine the scroll sequence
 50+ if( empty($this->m_params['perrow']) ){
 51+ $mAttribs['scroll'] = 1; // default 1
 52+ }else{
 53+ $mAttribs['scroll'] = $this->m_params['perrow'];
 54+ $mAttribs['visible'] = $this->m_params['perrow'];
 55+ }
 56+
3957 $ig->setAttributes( $mAttribs );
40 -
41 - // Require the javascript
 58+
 59+ // Load javascript module
4260 SMWOutputs::requireResource( 'ext.srf.jcarousel' );
4361 }
44 -
 62+
4563 // In case galleryformat = carousel, perrow should not be set
4664 if ( $this->m_params['perrow'] !== '' && $this->m_params['galleryformat'] !== 'carousel' ) {
4765 $ig->setPerRow( $this->m_params['perrow'] );
——@@ -177,7 +195,7 @@
178196 $ig->add( $imgTitle, $imgCaption );
179197
180198 // Only add real images (bug #5586)
181 - if ( $imgTitle->getNamespace() == NS_IMAGE && !$imgTitle->getDBkey() == null ) {
 199+ if ( $imgTitle->getNamespace() == NS_IMAGE && !is_null($imgTitle->getDBkey()) ) {
182200 $wgParser->mOutput->addImage( $imgTitle->getDBkey() );
183201 }
184202 }
——@@ -193,6 +211,7 @@
194212 $params = parent::getParameters();
195213
196214 if ( defined( 'MW_SUPPORTS_RESOURCE_MODULES' ) ) {
 215+ // Since 1.7.1
197216 $params['galleryformat'] = new Parameter( 'galleryformat', Parameter::TYPE_STRING, '' );
198217 $params['galleryformat']->setMessage( 'srf_paramdesc_galleryformat' );
199218 $params['galleryformat']->addCriteria( new CriterionInArray( 'carousel' ) );
Index: trunk/extensions/SemanticResultFormats/Gallery/resources/ext.srf.jcarousel.css
——@@ -1,228 +1,268 @@
2 -/**
 2+/**
33 * Adopted .jcarousel-skin-tango css for SRF Gallery Carousel module
44 *
55 * @licence: GNU GPL v3 or later
66 * @author: mwjames and others
77 *
8 - * @release: 0.1
 8+ * @release: 0.1.2
99 */
1010
1111 .jcarousel-skin-smw .jcarousel-container {
12 - background: #ffffff;
13 - border: 1px solid #eee;
 12+ background: #ffffff;
1413 }
1514
1615 .jcarousel-skin-smw .jcarousel-direction-rtl {
1716 direction: rtl;
1817 }
1918
 19+.jcarousel-list .jcarousel-list-horizontal {
 20+ margin-left:20px;
 21+}
 22+
2023 .jcarousel-skin-smw .jcarousel-container-horizontal {
21 - width: 99.6%;
22 - padding: 0px 1px;
 24+ width: 99.6%;
 25+ padding: 0px 1px;
 26+ margin-bottom:10px;
 27+ border: 1px solid #eee;
 28+
2329 }
2430
2531 .jcarousel-skin-smw .jcarousel-container-vertical {
26 - width: 175px;
27 - height: 345px;
28 - padding: 40px 20px;
 32+ width: 155px;
 33+ height: 365px;
 34+ padding: 23px 15px;
 35+ margin-left:10px;
 36+ margin-bottom:10px;
 37+ border: 1px solid #eee;
 38+ float:right;
2939 }
3040
3141 .jcarousel-skin-smw .jcarousel-clip {
32 - overflow: hidden;
 42+ overflow: hidden;
3343 }
3444
3545 .jcarousel-skin-smw .jcarousel-clip-horizontal {
36 - width: 97%;
37 - height: auto;
 46+ width: 97%;
 47+ height: auto;
3848 }
3949
4050 .jcarousel-skin-smw .jcarousel-clip-vertical {
41 - width: 175px;
42 - height: 345px;
 51+ width: 160px;
 52+ height: 355px;
4353 }
4454
4555 .jcarousel-skin-smw .jcarousel-item {
46 - width: auto;
47 - height: auto;
 56+ width: auto;
 57+ height: auto;
4858 }
4959
5060 .jcarousel-skin-smw .jcarousel-item-horizontal {
5161 margin-left: 23px;
52 - margin-right: 0px;
 62+ margin-right: 0px;
 63+ margin-top: 4px;
5364 }
5465
5566 .jcarousel-skin-smw .jcarousel-direction-rtl .jcarousel-item-horizontal {
56 - margin-left: 10px;
57 - margin-right: 0;
 67+ margin-left: 0px;
 68+ margin-right: 23px;
5869 }
5970
6071 .jcarousel-skin-smw .jcarousel-item-vertical {
61 - margin-bottom: 10px;
 72+ margin-bottom: 10px;
6273 }
6374
6475 .jcarousel-skin-smw .jcarousel-item-placeholder {
65 - background: #fff;
66 - color: #000;
 76+ background: #fff;
 77+ color: #000;
6778 }
6879
6980 /**
7081 * Horizontal Buttons
7182 */
7283 .jcarousel-skin-smw .jcarousel-next-horizontal {
73 - position: absolute;
74 - top: -1px;
75 - right: -1px;
76 - width: 32px;
77 - height: 32px;
78 - cursor: pointer;
79 - background: transparent url(images/next-horizontal.png) no-repeat 0 0;
80 - background-size:7px 11px;
 84+ position: absolute;
 85+ top: -1px;
 86+ right: -1px;
 87+ width: 32px;
 88+ height: 32px;
 89+ cursor: pointer;
 90+ background: transparent url(images/next-horizontal.png) no-repeat 0 0;
 91+ background-size:7px 11px;
8192 }
8293
8394 .jcarousel-skin-smw .jcarousel-direction-rtl .jcarousel-next-horizontal {
84 - left: 5px;
85 - right: auto;
86 - background-image: url(images/next-horizontal.png);
 95+ left: -1px;
 96+ right: auto;
 97+ background-image: url(images/next-horizontal.png);
8798 }
8899
89100 .jcarousel-skin-smw .jcarousel-next-horizontal:hover,
90101 .jcarousel-skin-smw .jcarousel-next-horizontal:focus {
91 - background-position: -32px 0;
 102+ background-position: -32px 0;
92103 }
93104
94105 .jcarousel-skin-smw .jcarousel-next-horizontal:active {
95 - background-position: -64px 0;
 106+ background-position: -64px 0;
96107 }
97108
98109 .jcarousel-skin-smw .jcarousel-next-disabled-horizontal,
99110 .jcarousel-skin-smw .jcarousel-next-disabled-horizontal:hover,
100111 .jcarousel-skin-smw .jcarousel-next-disabled-horizontal:focus,
101112 .jcarousel-skin-smw .jcarousel-next-disabled-horizontal:active {
102 - cursor: default;
103 - background-position: -96px 0;
 113+ cursor: default;
 114+ background-position: -96px 0;
104115 }
105116
106117 .jcarousel-skin-smw .jcarousel-prev-horizontal {
107 - position: absolute;
108 - top: -1px;
109 - left: -1px;
110 - width: 32px;
111 - height: 32px;
112 - cursor: pointer;
113 - background: transparent url(images/prev-horizontal.png) no-repeat 0 0;
114 - background-size:7px 11px;
 118+ position: absolute;
 119+ top: -1px;
 120+ left: -1px;
 121+ width: 32px;
 122+ height: 32px;
 123+ cursor: pointer;
 124+ background: transparent url(images/prev-horizontal.png) no-repeat 0 0;
 125+ background-size:7px 11px;
115126 }
116127
117128 .jcarousel-skin-smw .jcarousel-direction-rtl .jcarousel-prev-horizontal {
118 - left: auto;
119 - right: 5px;
120 - background-image: url(images/prev-horizontal.png);
 129+ left: auto;
 130+ right: -1px;
 131+ background-image: url(images/prev-horizontal.png);
121132 }
122133
123134 .jcarousel-skin-smw .jcarousel-prev-horizontal:hover,
124135 .jcarousel-skin-smw .jcarousel-prev-horizontal:focus {
125 - background-position: -32px 0;
 136+ background-position: -32px 0;
126137 }
127138
128139 .jcarousel-skin-smw .jcarousel-prev-horizontal:active {
129 - background-position: -64px 0;
 140+ background-position: -64px 0;
130141 }
131142
132143 .jcarousel-skin-smw .jcarousel-prev-disabled-horizontal,
133144 .jcarousel-skin-smw .jcarousel-prev-disabled-horizontal:hover,
134145 .jcarousel-skin-smw .jcarousel-prev-disabled-horizontal:focus,
135146 .jcarousel-skin-smw .jcarousel-prev-disabled-horizontal:active {
136 - cursor: default;
137 - background-position: -96px 0;
 147+ cursor: default;
 148+ background-position: -96px 0;
138149 }
139150
140 -.jcarousel-skin-smw .jcarousel-next-horizontal, .jcarousel-skin-smw .jcarousel-prev-horizontal {
141 - background-color: whiteSmoke;
142 - background-position: center;
143 - background-repeat: no-repeat;
144 - border: 1px solid gainsboro;
145 - color: #7B7B7B;
146 - cursor: pointer;
147 - font-size: 24px;
148 - font-weight: bold;
149 - height: 100%;
150 - line-height: 216px;
151 - text-align: center;
152 - text-decoration: none;
153 - position: absolute;
154 - width: 23px;
155 -
156 - border-radius: 2px 0 0 2px;
157 - -moz-border-radius: 2px 0 0 2px;
158 - -webkit-border-radius: 2px 0 0 2px;
159 -
 151+.jcarousel-skin-smw .jcarousel-next-horizontal,
 152+.jcarousel-skin-smw .jcarousel-prev-horizontal {
 153+ background-color: whiteSmoke;
 154+ background-position: center;
 155+ background-repeat: no-repeat;
 156+ border: 1px solid gainsboro;
 157+ color: #7B7B7B;
 158+ cursor: pointer;
 159+ font-size: 24px;
 160+ font-weight: bold;
 161+ height: 100%;
 162+ line-height: 216px;
 163+ text-align: center;
 164+ text-decoration: none;
 165+ position: absolute;
 166+ width: 23px;
 167+ border-radius: 2px 0 0 2px;
 168+ -moz-border-radius: 2px 0 0 2px;
 169+ -webkit-border-radius: 2px 0 0 2px;
160170 }
161171
162 -.jcarousel-skin-smw .jcarousel-next-horizontal:hover, .jcarousel-skin-smw .jcarousel-prev-horizontal:hover {
163 - background-color: #F8F8F8;
164 - border-color: #D6D6D6;
165 - box-shadow:inset 0 0 1px rgba(0, 0, 0, 0.1);
166 - color: #4B4B4B;
167 - text-decoration: none;
 172+.jcarousel-skin-smw .jcarousel-next-horizontal:hover,
 173+.jcarousel-skin-smw .jcarousel-prev-horizontal:hover {
 174+ background-color: #F8F8F8;
 175+ border-color: #D6D6D6;
 176+ box-shadow:inset 0 0 1px rgba(0, 0, 0, 0.1);
 177+ color: #4B4B4B;
 178+ text-decoration: none;
168179 }
169180
170181 /**
171182 * Vertical Buttons
172183 */
173184 .jcarousel-skin-smw .jcarousel-next-vertical {
174 - position: absolute;
175 - bottom: 5px;
176 - left: 43px;
177 - width: 32px;
178 - height: 32px;
179 - cursor: pointer;
180 - background: transparent url(images/next-vertical.png) no-repeat 0 0;
 185+ position: absolute;
 186+ bottom: -1px;
 187+ left: -1px;
 188+ width: 32px;
 189+ height: 32px;
 190+ cursor: pointer;
 191+ background: transparent url(images/next-vertical.png) no-repeat 0 0;
 192+ background-size:11px 7px;
 193+
181194 }
182195
183196 .jcarousel-skin-smw .jcarousel-next-vertical:hover,
184197 .jcarousel-skin-smw .jcarousel-next-vertical:focus {
185 - background-position: 0 -32px;
 198+ background-position: 0 -32px;
186199 }
187200
188201 .jcarousel-skin-smw .jcarousel-next-vertical:active {
189 - background-position: 0 -64px;
 202+ background-position: 0 -64px;
190203 }
191204
192205 .jcarousel-skin-smw .jcarousel-next-disabled-vertical,
193206 .jcarousel-skin-smw .jcarousel-next-disabled-vertical:hover,
194207 .jcarousel-skin-smw .jcarousel-next-disabled-vertical:focus,
195208 .jcarousel-skin-smw .jcarousel-next-disabled-vertical:active {
196 - cursor: default;
197 - background-position: 0 -96px;
 209+ cursor: default;
 210+ background-position: 0 -96px;
198211 }
199212
200213 .jcarousel-skin-smw .jcarousel-prev-vertical {
201 - position: absolute;
202 - top: 5px;
203 - left: 43px;
204 - width: 32px;
205 - height: 32px;
206 - cursor: pointer;
207 - background: transparent url(images/prev-vertical.png) no-repeat 0 0;
 214+ position: absolute;
 215+ top: -1px;
 216+ left: -1px;
 217+ width: 32px;
 218+ height: 32px;
 219+ cursor: pointer;
 220+ background: transparent url(images/prev-vertical.png) no-repeat 0 0;
 221+ background-size:11px 7px;
208222 }
209223
210224 .jcarousel-skin-smw .jcarousel-prev-vertical:hover,
211225 .jcarousel-skin-smw .jcarousel-prev-vertical:focus {
212 - background-position: 0 -32px;
 226+ background-position: 0 -32px;
213227 }
214228
215229 .jcarousel-skin-smw .jcarousel-prev-vertical:active {
216 - background-position: 0 -64px;
 230+ background-position: 0 -64px;
217231 }
218232
219233 .jcarousel-skin-smw .jcarousel-prev-disabled-vertical,
220234 .jcarousel-skin-smw .jcarousel-prev-disabled-vertical:hover,
221235 .jcarousel-skin-smw .jcarousel-prev-disabled-vertical:focus,
222236 .jcarousel-skin-smw .jcarousel-prev-disabled-vertical:active {
223 - cursor: default;
224 - background-position: 0 -96px;
 237+ cursor: default;
 238+ background-position: 0 -96px;
225239 }
226240
227 -.jcarousel-list .jcarousel-list-horizontal {
228 - margin-left:20px;
229 -}
 241+.jcarousel-skin-smw .jcarousel-next-vertical,
 242+.jcarousel-skin-smw .jcarousel-prev-vertical {
 243+ background-color: whiteSmoke;
 244+ background-position: center;
 245+ background-repeat: no-repeat;
 246+ border: 1px solid gainsboro;
 247+ color: #7B7B7B;
 248+ cursor: pointer;
 249+ font-size: 24px;
 250+ font-weight: bold;
 251+ width: 100%;
 252+ line-height: 216px;
 253+ text-align: center;
 254+ text-decoration: none;
 255+ position: absolute;
 256+ height: 23px;
 257+ border-radius: 2px 0 0 2px;
 258+ -moz-border-radius: 2px 0 0 2px;
 259+ -webkit-border-radius: 2px 0 0 2px;
 260+}
 261+
 262+.jcarousel-skin-smw .jcarousel-next-vertical:hover,
 263+.jcarousel-skin-smw .jcarousel-prev-vertical:hover {
 264+ background-color: #F8F8F8;
 265+ border-color: #D6D6D6;
 266+ box-shadow:inset 0 0 1px rgba(0, 0, 0, 0.1);
 267+ color: #4B4B4B;
 268+ text-decoration: none;
 269+}
\ No newline at end of file
Index: trunk/extensions/SemanticResultFormats/Gallery/resources/ext.srf.jcarousel.js
——@@ -11,12 +11,24 @@
1212
1313 $( document ).ready( function() {
1414
 15+ var v_visible = parseInt($( '#carousel' ).attr( 'visible' ) ),
 16+ v_scroll = parseInt($( '#carousel' ).attr( 'scroll' ) ),
 17+ v_directionality = $( '#carousel' ).attr( 'directionality' ),
 18+ v_orientation = $( '#carousel' ).attr( 'orientation' ),
 19+ v_wrap = $( '#carousel' ).attr( 'wrap' ),
 20+ v_vertical = v_orientation === 'vertical',
 21+ v_rtl = v_directionality === 'rtl';
 22+
1523 // Display carousel only after js is loaded and ready otherwise display=none
1624 $( '#carousel').show();
1725
1826 // Call the jcarousel plug-in
1927 $( '#carousel' ).jcarousel( {
20 - wrap: 'circular'
 28+ scroll: v_scroll, // Number of items to be scrolled
 29+ visible: v_visible, // calculated and set visible elements
 30+ wrap: 'circular', // Whether to wrap at the first/last item (Options are "first", "last", "both" or "circular")
 31+ vertical: v_vertical, // Whether the carousel appears in horizontal or vertical orientation
 32+ rtl: v_rtl // Directionality
2133 } );
2234
2335 } );

Follow-up revisions

RevisionCommit summaryAuthorDate
r112204rem BOM, follow up r112201jeroendedauw14:03, 23 February 2012
r112205Removed BOM and stylize, ping r112201nikerabbit14:09, 23 February 2012
r112215bug 34411 patch by MWJames, somewhat modifiedjeroendedauw16:28, 23 February 2012

Comments

#Comment by Nikerabbit (talk | contribs)   13:25, 23 February 2012

BOM added to PHP/JS files.

#Comment by Nikerabbit (talk | contribs)   13:26, 23 February 2012

PHP and CSS files*

#Comment by Jeroen De Dauw (talk | contribs)   14:04, 23 February 2012

Maybe I failed, but did not find it in SRF_Gallery.php

#Comment by MWJames (talk | contribs)   13:30, 23 February 2012

For all non-developers what is the issue here. Since I have to provided a new patch anyway please explain it plain and simple what needs to be fixed because the above comments don't tell me anything.

Status & tagging log