r95448 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r95447‎ | r95448 | r95449 >
Date:21:37, 24 August 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
jquery.spinner: Fix small issues
(Follows-up r95445)
* Using the same spinner as before, preserving svn history (20px vs. 16px)
* Adding a line-break at the end of file and changing indentation per our conventions
* Adjusting doc to reflect the plugin itself instead of what it replaces.
* Using shorthand utility in jQuery (no / and using the attr-object as second argument)
* Adding @return comment to $.fn.injectSpinner
* Adding return statement to $.removerSpinner (returning a jQuery object of the element). Could be useful, but better than not having a return value at all.

JSHint:
* Adding parentheses around the class object property, otherwise it may be interpreted as a class operator which breaks the object.
Modified paths:
  • /trunk/phase3/resources/jquery/images/spinner.gif (replaced) (history)
  • /trunk/phase3/resources/jquery/jquery.spinner.css (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.spinner.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/images/spinner.gif
Cannot display: file marked as a binary type.
svn:mime-type = image/gif
Index: trunk/phase3/resources/jquery/jquery.spinner.css
@@ -1,8 +1,8 @@
2 -.loading-spinner {
 2+.mw-spinner {
33 /* @embed */
4 - background: transparent url('images/spinner.gif');
5 - height: 16px;
6 - width: 16px;
 4+ background: transparent url(images/spinner.gif);
 5+ height: 20px;
 6+ width: 20px;
77 display: inline-block;
88 vertical-align: middle;
99 }
\ No newline at end of file
Index: trunk/phase3/resources/jquery/jquery.spinner.js
@@ -1,42 +1,45 @@
22 /**
3 - * Functions to replace injectSpinner which makes img tags with spinners
 3+ * jQuery spinner
 4+ *
 5+ * Simple jQuery plugin to create, inject and remove spinners.
46 */
57 ( function( $ ) {
68
79 $.extend( {
810 /**
9 - * Creates a spinner element
 11+ * Creates a spinner element.
1012 *
11 - * @param id String id of the spinner
12 - * @return jQuery spinner
 13+ * @param id {String} id of the spinner
 14+ * @return {jQuery} spinner
1315 */
1416 createSpinner: function( id ) {
15 - return $( '<div/>' )
16 - .attr({
17 - id: 'mw-spinner-' + id,
18 - class: 'loading-spinner',
19 - title: '...',
20 - alt: '...'
21 - });
 17+ return $( '<div>' ).attr( {
 18+ id: 'mw-spinner-' + id,
 19+ 'class': 'mw-spinner',
 20+ title: '...',
 21+ alt: '...'
 22+ } );
2223 },
2324
2425 /**
25 - * Removes a spinner element
 26+ * Removes a spinner element.
2627 *
27 - * @param id
 28+ * @param id {String}
 29+ * @return {jQuery} spinner
2830 */
2931 removeSpinner: function( id ) {
30 - $( '#mw-spinner-' + id ).remove();
 32+ return $( '#mw-spinner-' + id ).remove();
3133 }
3234 } );
3335
3436 /**
35 - * Injects a spinner after the given objects
 37+ * Injects a spinner after the elements in the jQuery collection.
3638 *
3739 * @param id String id of the spinner
 40+ * @return {jQuery}
3841 */
3942 $.fn.injectSpinner = function( id ) {
4043 return this.after( $.createSpinner( id ) );
4144 };
4245
43 -} )( jQuery );
\ No newline at end of file
 46+} )( jQuery );

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r95445(bug 30499) Create jQuery replacements for injectSpinner and removeSpinnerjohnduhart21:08, 24 August 2011

Comments

#Comment by Krinkle (talk | contribs)   21:43, 24 August 2011

Commit-msg says "using the attr-object as second argument", I decided not to do that as the gain in space is minimal and jsperf shows that it's actually a bit slower. Left it the way it was.

Status & tagging log