User "Catrope" changed the status of MediaWiki.r96309. Old Status: new New Status: fixme
User "Catrope" also posted a comment on MediaWiki.r96309. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/96309#c22253 Commit summary: [ResourceLoader2] Gadget manager (start of ajax editor) * Add column to SpecialGadgetManager front-end for last modified timestamp. * Use getTitleMessage. Logic is now centralized (yay r95970). * Expose gadget internal name to JavaScript via data- attribute. * Expose array of all rights to JavaScript via OutputPage's getJSVars hook. * Only load ajax editor if user is allowed to edit NS_GADGET_DEFINITION. * Introduce three resource modules: ** ext.gadgetmanager.api: Group of (partially stub) function to interact with the API. Getting gadget metadata, list of categories, save gadget, delete gadget etc. ** ext.gadgetmanager.ui: Binds event handlers to the table outputted by PHP and takes care of the form/dialog generation and the autocompletion of all input fields through various apis and arrays. ** jquery.createPropcloud: jQuery plugin written from scratch for the gadget manager. Turns an input field into a properties "cloud" with autocompletion and visual bubbles to remove items. TODO: * ext.gadgetmanager.api ** Finish stub functions for saving and deleting a gadget. * ext.gadgetmanager.ui ** Nofication system ** Add "Create" tab to the special page, which just brings up a regular modify-form except that it will feature a title-input field. ** Add dynamic "(new category)" option in the category dropdown of the gadget editor. * SpecialGadgetManager.php ** generateGadgetView (Follows-up r95818) Comment: None of the JS files added in this revision alias jQuery to $ in their closures. <pre> + return $( '<span class="' + o.prefix + 'prop"></span>' ) </pre> Eww. Generic functions should always escape their arguments. Never make the caller responsible for escaping. This pattern is repeated throughout the plugin; only one line uses <code>addClass()</code>. <pre> + * <span editor="jquery-prop"> </pre> Weird typo? <pre> + * - onAdd {Function} Callback when an item is added + * - onRemove {Function} Callback when an item is deleted </pre> You should document the parameter lists of these callbacks. <pre> + o.prefix = o.prefix + '-'; </pre> This is why mankind invented <code>+=</code> ;) . Also, would it be that unreasonable to require that the caller put in the dash? That seems much more logical to me. <pre> + // Prevent duplicate values + if ( o.props.indexOf( val ) === -1 ) { + $container.append( newPropHtml( val, o ) ); + o.onAdd( val ); + } </pre> Doesn't this fail to prevent duplicate added values, because <code>val</code> isn't added to <code>o.props</code> ? The way it looks like to me at a quick glance is that if you've got <code>o.props = ['foo','bar']</code> it won't let you add foo or bar, but it will let you add baz twice. <pre> * @var {Object|Null} If cache, object with category ids and the member counts */ </pre> That sentence doesn't make much sense to me. It's also nice to document the structure of these things, e.g. <code>// { gadgetID: metadataObject }</code> I don't really understand the architectural decision to have a single callback with a parameter that specifies success or error rather than having separate callbacks for both cases (the latter pattern is used a lot in UploadWizard). When would the success callback and the error callback ever share code? You're also using your pattern inconsistently: <code>getGadgetMetadata()</code> and <code>getGadgetCategories()</code> use <code>callback( {}, 'error' )</code> but <code>doModifyGadget()</code> uses <code>callback( 'error', msg )</code> . <pre> +.mw-gadgetmanager-propcontainer {} [...] +.mw-gadgetmanager-prop-label {} </pre> ??? <pre> <legend>Module properties</legend>\ </pre> Hardcoded English. Isn't this <code>jquery.localize</code>-able? <pre> * @param id {String} * @param displayTitle {String} */ startGadgetEditor: function( gadget ) { </pre> Documentation doesn't match the function signature. <pre> alert( "Save result: \n- status: " + status + "\n- msg: " + msg ); </pre> alert()? Really? :D <pre> var suggestions = $.map( json.query.gadgetpages, function( val, i ) { return val.pagename; }); suggestions = suggestions.splice( 0, suggestLimit ); </pre> Wouldn't it make more sense to do the splice before the map, to eliminate wasted effort? There's a lot of code duplication between the <code>createPropCloud()</code> calls for JS and CSS suggestions, and less so between the others. At least the former looks like it could be factored out. <pre> $form.find( '#mw-gadgetmanager-input-category' ).append( function() { </pre> I had no idea that worked; cool feature. <pre> + $html .= '<th>' . wfMessage( 'gadgetmanager-tablehead-lastmod' )->escaped() . '</tr>'; </pre> No closing th tag. Probably cool in HTML5 mode but not so much in other modes. <pre> + $definitionTitle = Title::newFromText( $gadget->getName() . '.js', NS_GADGET_DEFINITION ); </pre> This doesn't work correctly if you happen to have a gadget called <code>Talk:Foo</code>. Yes, I know that's idiotic, but code needs to be idiot-proof when it comes to dealing with user-supplied data. Use <code>Title::makeTitleSafe( NS_GADGET_DEFINITION, $str )</code> instead. Marking fixme because of the escaping, closing th tag and newFromText issues. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
