r98080 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r98079‎ | r98080 | r98081 >
Date:19:58, 25 September 2011
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
MoodBar: Write the filter state to a cookie and read it on page load. This still needs AJAX filtering to be implemented and the on-page-load state to actually take effect.
Modified paths:
  • /trunk/extensions/MoodBar/modules/ext.moodBar.dashboard/ext.moodBar.dashboard.js (modified) (history)

Diff [purge]

Index: trunk/extensions/MoodBar/modules/ext.moodBar.dashboard/ext.moodBar.dashboard.js
@@ -1,10 +1,48 @@
22 jQuery( function( $ ) {
 3+ function getSelectedTypes() {
 4+ var types = [];
 5+ $( '#fbd-filters-type-praise, #fbd-filters-type-confusion, #fbd-filters-type-issues' ).each( function() {
 6+ if ( $(this).prop( 'checked' ) ) {
 7+ types.push( $(this).val() );
 8+ }
 9+ } );
 10+ return types;
 11+ }
 12+
 13+ function setCookies() {
 14+ $.cookie( 'moodbar-feedback-types', getSelectedTypes().join( '|' ), { 'path': '/', 'expires': 7 } );
 15+ $.cookie( 'moodbar-feedback-username', $( '#fbd-filters-username' ).val(), { 'path': '/', 'expires': 7 } );
 16+ }
 17+
 18+ function loadFromCookies() {
 19+ var cookieTypes = $.cookie( 'moodbar-feedback-types' );
 20+ $username = $( '#fbd-filters-username' );
 21+ if ( $username.val() == '' ) {
 22+ var cookieUsername = $.cookie( 'moodbar-feedback-username' );
 23+ if ( cookieUsername != '' ) {
 24+ $username.val( cookieUsername );
 25+ }
 26+ }
 27+
 28+ if ( cookieTypes ) {
 29+ cookieTypes = '|' + cookieTypes;
 30+ $( '#fbd-filters-type-praise, #fbd-filters-type-confusion, #fbd-filters-type-issues' ).each( function() {
 31+ if ( !$(this).prop( 'checked' ) && cookieTypes.indexOf( '|' + $(this).val() ) != -1 ) {
 32+ $(this).prop( 'checked', true );
 33+ }
 34+ } );
 35+ }
 36+ }
 37+
 38+ $( '#fbd-filters-set' ).click( setCookies );
 39+ loadFromCookies();
 40+
341 $( '#fbd-list-more').children( 'a' ).click( function( e ) {
442 e.preventDefault();
543
644 var limit = 20,
745 username = $( '#fbd-filters-username' ).val(),
8 - types = [],
 46+ types = getSelectedTypes(),
947 reqData;
1048
1149 // Hide the "More" link and put in a spinner
@@ -22,11 +60,6 @@
2361 'mbclimit': limit + 2, // we drop the first and last result
2462 'mbccontinue': $( '#fbd-list').find( 'li:last' ).data( 'mbccontinue' )
2563 };
26 - $( '#fbd-filters-type-praise, #fbd-filters-type-confusion, #fbd-filters-type-issues' ).each( function() {
27 - if ( $(this).prop( 'checked' ) ) {
28 - types.push( $(this).val() );
29 - }
30 - } );
3164 if ( types.length ) {
3265 reqData['mbctype'] = types.join( '|' );
3366 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r98300Fix stupid typo in r98080, r98116catrope10:55, 28 September 2011

Comments

#Comment by Krinkle (talk | contribs)   01:52, 28 September 2011
+		var	cookieTypes = $.cookie( 'moodbar-feedback-types' );
+			$username = $( '#fbd-filters-username' );

$username goes global. This is further ruined by r98116 with a syntax error when the semi colon became a comma.

#Comment by Catrope (talk | contribs)   10:55, 28 September 2011

foo = 3, bar = 4; is not a syntax error. The code ran fine in FF 6.0.1.

Good catch, though, fixed in r98300.

#Comment by Krinkle (talk | contribs)   18:08, 28 September 2011

I always thought comma separated assignments only work in var statements (or at least are supposed to), not for (implied) global variables or re-defining (local) variables. It works in FF6  - and - in Chrome 13 as well apparently. Not sure about IE or what the specification says about this. Anyway, interesting :)

#Comment by Catrope (talk | contribs)   10:08, 29 September 2011

They work in for loops, too, even without var, remember? I haven't read the ECMAScript spec but I would expect that it defines the comma operator very similar to C's, in that a, b is an expression that first evaluates a, discards the result, then evaluates b, and returns that result. a, b, c would associate as a, (b, c) so a would be evaluated first, then b, then c, and c would be returned.

In other words, x = (y = 3, z = 4); is equivalent to y = 3; x = (z = 4);. Make sense? :D

#Comment by Tim Starling (talk | contribs)   06:22, 30 December 2011

The ECMAScript spec is not a pleasant thing to read. Here's an easier option:

It says it's in ECMA-262 and JavaScript 1.0. Of course anyone actually using the comma operator outside of the increment part of a "for" loop should probably be shot.

#Comment by Catrope (talk | contribs)   14:30, 3 January 2012

I was just assuming the comma would work the same way as it does in C, which seems to be true. And yeah, if someone intentionally uses a comma in any other place than a for initialization, for increment, or var statement, they don't deserve my sympathy.

Status & tagging log