r95445 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r95444‎ | r95445 | r95446 >
Date:21:08, 24 August 2011
Author:johnduhart
Status:resolved (Comments)
Tags:
Comment:
(bug 30499) Create jQuery replacements for injectSpinner and removeSpinner
Modified paths:
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/jquery/images (added) (history)
  • /trunk/phase3/resources/jquery/images/spinner.gif (added) (history)
  • /trunk/phase3/resources/jquery/jquery.spinner.css (added) (history)
  • /trunk/phase3/resources/jquery/jquery.spinner.js (added) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/images/spinner.gif
Cannot display: file marked as a binary type.
svn:mime-type = image/gif
Property changes on: trunk/phase3/resources/jquery/images/spinner.gif
___________________________________________________________________
Added: svn:mime-type
11 + image/gif
Index: trunk/phase3/resources/jquery/jquery.spinner.css
@@ -0,0 +1,8 @@
 2+.loading-spinner {
 3+ /* @embed */
 4+ background: transparent url('images/spinner.gif');
 5+ height: 16px;
 6+ width: 16px;
 7+ display: inline-block;
 8+ vertical-align: middle;
 9+}
\ No newline at end of file
Property changes on: trunk/phase3/resources/jquery/jquery.spinner.css
___________________________________________________________________
Added: svn:eol-style
110 + native
Index: trunk/phase3/resources/jquery/jquery.spinner.js
@@ -0,0 +1,42 @@
 2+/**
 3+ * Functions to replace injectSpinner which makes img tags with spinners
 4+ */
 5+( function( $ ) {
 6+
 7+$.extend( {
 8+ /**
 9+ * Creates a spinner element
 10+ *
 11+ * @param id String id of the spinner
 12+ * @return jQuery spinner
 13+ */
 14+ createSpinner: function( id ) {
 15+ return $( '<div/>' )
 16+ .attr({
 17+ id: 'mw-spinner-' + id,
 18+ class: 'loading-spinner',
 19+ title: '...',
 20+ alt: '...'
 21+ });
 22+ },
 23+
 24+ /**
 25+ * Removes a spinner element
 26+ *
 27+ * @param id
 28+ */
 29+ removeSpinner: function( id ) {
 30+ $( '#mw-spinner-' + id ).remove();
 31+ }
 32+} );
 33+
 34+/**
 35+ * Injects a spinner after the given objects
 36+ *
 37+ * @param id String id of the spinner
 38+ */
 39+$.fn.injectSpinner = function( id ) {
 40+ return this.after( $.createSpinner( id ) );
 41+};
 42+
 43+} )( jQuery );
\ No newline at end of file
Property changes on: trunk/phase3/resources/jquery/jquery.spinner.js
___________________________________________________________________
Added: svn:eol-style
144 + native
Index: trunk/phase3/resources/Resources.php
@@ -156,6 +156,10 @@
157157 'jquery.qunit.completenessTest' => array(
158158 'scripts' => 'resources/jquery/jquery.qunit.completenessTest.js',
159159 ),
 160+ 'jquery.spinner' => array(
 161+ 'scripts' => 'resources/jquery/jquery.spinner.js',
 162+ 'styles' => 'resources/jquery/jquery.spinner.css',
 163+ ),
160164 'jquery.suggestions' => array(
161165 'scripts' => 'resources/jquery/jquery.suggestions.js',
162166 'styles' => 'resources/jquery/jquery.suggestions.css',

Follow-up revisions

RevisionCommit summaryAuthorDate
r95448jquery.spinner: Fix small issues...krinkle21:37, 24 August 2011
r95450Follow-up r95445, fix for IE6-7 no supporting display-inline. Removes alt att...johnduhart21:55, 24 August 2011

Comments

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

Please note that inline-block is not supported in IE6 and IE7. In the legacy script this wasn't an issue because it used an ‎<img>, now that it is a ‎<div> it becomes an issue. Either use an ‎<img> or make sure it works in IE6 / IE7 without becoming a block level element (ie-star-hack + hasLayout).

#Comment by Johnduhart (talk | contribs)   21:31, 24 August 2011

Roan had an issue with using an image with a spinner because if the user's connection was slow, loading the image would take too long and not be shown. Here it's embedded in the CSS.

As for the inline-block I'll have to find a workaround. Marking fixme, will follow-up

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

Yep, I agree a ‎<div> is better here.

#Comment by MaxSem (talk | contribs)   05:55, 25 August 2011

Why not use already existing skins/common/images/spinner.gif ?

#Comment by Krinkle (talk | contribs)   11:50, 25 August 2011

I had the same thought yesterday, see r95448.

Status & tagging log