User:Krinkle/Extension review/FlaggedRevs (front-end)

Review form

aka #mw-fr-reviewform and review.js

Note that the "Bugs' and "User experience" section is most if not completely about things that are not recently added, but I took note of it while at it. The review of recently added code is under "Code structure and quality"

In no particular order

Bugs

• "Cancel" goes to history.back(). So if one go from A to B and B has a review form on in, enter some details (or don't) and click that button, we're taken back to A for no apparent reason.

User experience

• When clicking "advertise" the sentence is swapped for a yellow-marked line. However when clicking "(de-advertise)" it does not change back. Note that neither state seem to affect the review action, it is unclear to me as (a user what) this feature is supposed to do or prevent.
• If the latest revision to the page is already reviewed (i.e. by the currently logged-in user) the form looks somewhat broken. I can click (de-)advertise and type a reason but the "Accept"-button is greyed out. The form legend says "Re-review revision". Perhaps don't show "Accept" when re-reviewing (just like "Unaccept" is not shown when doing a regular review) ?

Code structure and quality

• Use file naming convention for extensions (e.g. ext.flaggedRevs.basic.css, ext.flaggedRevs.advanced.css, ext.flaggedRevs.review.js)
• document.title = mw.msg('actionfailed'); this is removing the current page name and site name from the title. Try to avoid changing the document.title, or if really needed use the message defined under "pagetitle" to construct it. (e.g. document.title = mw.msg('pagetitle', mw.msg('actionfailed')).replace(new RegExp('{{SITENAME}}','g'), wgSiteName);). Or if you feel like it, solve bugzilla:31258 and use that.
• You may refer to window.mediaWiki with just "mw". It is a global alias that can be used without risk (in contrary to $, which should be localized aliased for now)
• Wrap js files in an immediately invoked function expression closure, to make sure that your scope really is local and not leaking from or to other modules.
• $ and jQuery are used mixed. To use $, either locally make an alias to jQuery, or use $ as first argument in the closure (see previous point) and call it with "jQuery" (see example).
• Don't use global variables other than jQuery and mw. Put FlaggedRevsReview into mw.FlaggedRevs.review or mw.FlaggedRevsReview
• Configuration variables exposed to javascript (wgFlaggedRevsParams, wgScriptPath) should be accessed through mw.config, especially now that wikis can disable legacy globals ($wgLegacyJavaScriptGlobals ) - in which case referring said global, JS will throw a ReferenceError.
• Although is a rare edgecase, when using index.php or api.php always take wgScriptExtension into account. You can do this automatically by using mw.util.wikiScript() and mw.util.wikiScript( 'api' ).
• Don't use index.php?action=ajax. Although it can be a hassle to rewrite, in the long term this should be an ApiAction module.
• $form .= Html::hidden( 'refid', $priorRevId, array( 'id' => 'mw-fr-input-refid' ) ) . "\n";
var oRevId = $('#mw-fr-input-refid') ? $('#mw-fr-input-refid').val() : 0;
jQuery.ajax({ .., data: { previd: oRevId, .. }, .. });
  Use the MakeGlobalVariablesScript hook instead to send this data into mw.config.
• Other stuff:
  • Code conventions and good practices (strict comparison, curly braces around all blocks, dot notation for object members, single quotes, whitespace, combining var statements, ..)
  • JSHint.com, recommended settings:
    • Warn: All on, except ("About unsafe comparisons", "When code is not in strict mode", "When variable is undefined")
    • Assume: Just browser

General

flaggedrevs.js/flaggedrevs.css

todo