r112215 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r112214‎ | r112215 | r112216 >
Date:16:28, 23 February 2012
Author:jeroendedauw
Status:deferred (Comments)
Tags:
Comment:
bug 34411 patch by MWJames, somewhat modified
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
@@ -32,25 +32,24 @@
3333 $ig->setCaption( $this->mIntro ); // set caption to IQ header
3434
3535 if ( $this->m_params['galleryformat'] == 'carousel' ) {
 36+ static $carouselNr = 0;
 37+
3638 // Set attributes for jcarousel
37 - $mAttribs['id'] = 'carousel';
38 - $mAttribs['class'] = 'jcarousel-skin-smw';
 39+ $attribs = array(
 40+ 'id' => 'carousel' . ++$carouselNr,
 41+ 'class' => 'jcarousel jcarousel-skin-smw',
 42+ 'style' => 'display:none;', // Avoid js loading issues by not displaying anything until js is able to do so.
 43+ 'wrap' => 'both', // Whether to wrap at the first/last item (or both) and jump back to the start/end.
 44+ 'vertical' => 'false', // Orientation: vertical = false means horizontal
 45+ 'rtl' => 'false', // Directionality: rtl = false means ltr
 46+ );
3947
40 - // Aoid js loading issues by not displaying anything until js is able to do so
41 - $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
 48+ // Use perrow parameter to determine the scroll sequence.
5049 if ( empty( $this->m_params['perrow'] ) ) {
51 - $mAttribs['scroll'] = 1; // default 1
 50+ $attribs['scroll'] = 1; // default 1
5251 } else {
53 - $mAttribs['scroll'] = $this->m_params['perrow'];
54 - $mAttribs['visible'] = $this->m_params['perrow'];
 52+ $attribs['scroll'] = $this->m_params['perrow'];
 53+ $attribs['visible'] = $this->m_params['perrow'];
5554 }
5655
5756 $ig->setAttributes( $mAttribs );
Index: trunk/extensions/SemanticResultFormats/Gallery/resources/ext.srf.jcarousel.css
@@ -4,7 +4,7 @@
55 * @licence: GNU GPL v3 or later
66 * @author: mwjames and others
77 *
8 - * @release: 0.1.2
 8+ * @release: 0.1.3
99 */
1010
1111 .jcarousel-skin-smw .jcarousel-container {
Index: trunk/extensions/SemanticResultFormats/Gallery/resources/ext.srf.jcarousel.js
@@ -4,32 +4,34 @@
55 * @licence: GNU GPL v3 or later
66 * @author: mwjames
77 *
8 - * @release: 0.1
 8+ * @release: 0.1.3
99 */
1010
1111 (function( $ ) {
1212
1313 $( document ).ready( function() {
14 -
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';
2214
23 - // Display carousel only after js is loaded and ready otherwise display=none
24 - $( '#carousel').show();
25 -
26 - // Call the jcarousel plug-in
27 - $( '#carousel' ).jcarousel( {
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
33 - } );
 15+ // Bind individual elements containing class jcarousel as the plug-in
 16+ // requires different id's
 17+ $(".jcarousel").each(function() {
 18+ var galleryId = "#" + $(this).closest(".jcarousel").attr("id"),
 19+ v_vertical = ( $( this ).attr( 'vertical' ) === 'true'), // Value is either true or false
 20+ v_rtl = ( $( this ).attr( 'rtl' ) === 'true'); // Value is either true or false
 21+ //console.log(galleryId);
 22+
 23+ // Display carousel only after js is loaded and is ready otherwise display=none
 24+ $( this ).show();
 25+
 26+ // Call the jcarousel plug-in
 27+ $( this ).jcarousel({
 28+ scroll: parseInt($( this ).attr( 'scroll' ) ), // Number of items to be scrolled
 29+ visible: parseInt($( this ).attr( 'visible' ) ), // calculated and set visible elements
 30+ wrap: $( this ).attr( 'wrap' ), // 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
 33+ } );
 34+
 35+ } );
3436
3537 } );
3638

Follow-up revisions

RevisionCommit summaryAuthorDate
r112351Follow up to r112215;jeroendedauw20:28, 24 February 2012
r112362Follow up to r112215;jeroendedauw22:14, 24 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r112201slightly modified patch by MWJames from bug 34411jeroendedauw13:16, 23 February 2012

Comments

#Comment by Nikerabbit (talk | contribs)   07:59, 24 February 2012

parseInt should have second parameter to always use base 10. Not fan of the custom html attributes, but likely not a problem.

galleryId is unused. v_vertical and v_rtl leak into global scope. I'd pull $( this ) into variable instead of calling it over and over again.

#Comment by Jeroen De Dauw (talk | contribs)   20:30, 24 February 2012

Stuff not remarked upon below taken care off in follow up.

> Not fan of the custom html attributes, but likely not a problem.

What do you suggest doing instead?

> v_vertical and v_rtl leak into global scope

Huh??? They are in a closure... How are they getting into global scope, and how would you fix this?

#Comment by Nikerabbit (talk | contribs)   21:59, 24 February 2012

Maybe you could use data- attributes from html5. It's basically the same but html validators wont complain about them and in recent jQuery you can use .data() to access them.

Variables in JavaScript are global unless introduced with var.

var x = ...;
#Comment by Jeroen De Dauw (talk | contribs)   22:05, 24 February 2012

> Maybe you could use data- attributes from html5

Right, this is what I usually do, so happy to see you don't object to that :)

> Variables in JavaScript are global unless introduced with var.

But these ones are introduced w/ var...

#Comment by Nikerabbit (talk | contribs)   22:06, 24 February 2012

Yes you are correct. I hate the colon vs. semicolon difference though. Easy to mistake, especially with comments on the lines.

#Comment by Jeroen De Dauw (talk | contribs)   22:09, 24 February 2012

jslint goes mad at you when you repeat the var stuff, and I've seen this approach recommended at various places, so guess it must be better? I don't care much though.

#Comment by Nikerabbit (talk | contribs)   22:11, 24 February 2012

Not so much about the practice but about the design of the language. Maybe little extra indentation would make it clearer though.

Status & tagging log