Page MenuHomePhabricator

Review user feedback gadget code
Closed, ResolvedPublic

Description

Need help from someone with experience working with Gadgets to review code for user feedback gadget that we will pilot on MediaWiki (to begin with mostly in the API namespace) or Wikitech.

Source for the gadget both JS & CSS on test wiki:
https://test.wikipedia.org/wiki/MediaWiki:Gadget-userfeedback.js
https://test.wikipedia.org/wiki/MediaWiki:Gadget-userfeedback.css

View User feedback form design here https://github.com/srish/wiki-user-feedback

(already done testing on local wiki with eventlogging devserver setup)

Event Timeline

srishakatux triaged this task as Medium priority.Jan 15 2019, 1:16 AM
srishakatux created this task.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 15 2019, 1:16 AM
  • As part of converting from a user script to a gadget, you can (and should):
    • Remove the mw.loader.load() call that loads the CSS. It already gets loaded because it's in the gadget definition
    • Instead of wrapping everything in mw.loader.using( 'ext.eventLogging' ), define this dependency in the gadget definition, like so: userfeedback[ResourceLoader|dependencies=ext.eventLogging]|userfeedback.js|userfeedback.css
  • Confusingly, wgTitle is not a very useful variable, and mw.Title.newFromText( title, namespaceNum ) is not the best way to make a Title object for the current page. Instead, I recommend mw.Title.newFromText( mw.config.get( 'wgPageName' ) ) (and if you do this, you can get rid of the namespaceNum variable too)
  • When building HTML using string concatenation, you have to escape attributes that could contain special characters. There's only one instance of that in this code: "<a href='" + talkPageUrl + "' target='_blank'>blah</a>" should be "<a href="' + mw.html.escape( talkPageUrl ) + "' target='_blank'>blah</a>
    • Ideally, you wouldn't build HTML with string concatenation at all. jQuery lets you build DOM nodes programmatically (but that's a little bit more verbose, see below)
  • IDs should be prefixed to avoid naming collisions. thumbs-up is too generic of an ID, I suggest something like mw-userfeedback-thumbs-up (and something similar for the others). The class name thumbs is also pretty generic, but that's not as much of a problem because all your CSS rules for it are limited to within the #doc-feedback-form div; ideally, though, this would be prefixed too.
  • Our coding conventions say to:
      • use === instead of == (but !== is correct)
    • use 'single quotes' for strings where possible (this code uses a mix of 'single quoted' and "double quoted" strings)

Programmatic building of DOM nodes with jQuery (this is more of a bonus thing):

// To build a link with a dynamic href, you can do this:
 $( '#doc-feedback-form' ).append(
    $( '<span' ).text( 'blah blah blah' ),
    $( '<a>' ).attr( { href: talkPageUrl, target: '_blank' } ).text( 'click here' )
);

// You can also attach click handlers before attaching nodes, so then you don't have to find them back later by their ID:
$('#doc-feedback-form').append( $( '<a>' )
    .attr( { id: 'thumbs-up' } )
    .addClass( 'thumbs' )
    .click( function () {... } )
);

// You can also store nodes in variables so you don't have to find them back by their ID later:
var $form = $( '<div>' ).attr( { id: 'doc-feedback-form' } );
$( display ).after( $form );
$form.append( ... );
srishakatux closed this task as Resolved.Feb 14 2019, 10:22 PM

Thanks a lot @Catrope!