launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Review: Approve code

I think this is a good start. It is must better than what users are seeing now. I have a few remarks.

> === modified file 'lib/lp/app/javascript/formoverlay/formoverlay.js'
> --- lib/lp/app/javascript/formoverlay/formoverlay.js	2012-06-26 00:15:11 +0000
> +++ lib/lp/app/javascript/formoverlay/formoverlay.js	2012-07-26 11:02:24 +0000
> @@ -492,18 +492,19 @@
>       * @method showError
>       */
>      showError: function(error_msgs){
> +        var error_content;
>          if (Y.Lang.isString(error_msgs)) {
> -            error_msgs = [error_msgs];
> +            error_content = Y.Node.create('<p></p>').set('text', error_msgs);
> +        } else {
> +            error_content = Y.Node.create(
> +                '<p>The following errors were encountered:</p><ul></ul>');
> +            var error_list = error_content.one('ul');
> +            Y.each(error_msgs, function(error_msg){
> +                error_list.appendChild(
> +                    Y.Node.create('<li></li>').set('text', error_msg));
> +            });

This iteration is expensive. repeated appends also call repeated refreshes
You can build the nodes individually, or pass a string to update the UI once.

            var li_nodes = new Y.NodeList([]);
            Y.each(error_msgs, function(error_msg){
                li_nodes.push(
                    Y.Node.create('<li></li>').set('text', error_msg));
            });
            error_list.append(li_nodes);

> === modified file 'lib/lp/bugs/javascript/duplicates.js'
> --- lib/lp/bugs/javascript/duplicates.js	2012-07-25 00:31:04 +0000
> +++ lib/lp/bugs/javascript/duplicates.js	2012-07-26 11:02:24 +0000
> @@ -13,10 +13,10 @@
>  // Overlay related vars.
>  var submit_button_html =
>      '<button type="submit" name="field.actions.change" ' +
> -    'value="Change" class="lazr-pos lazr-btn" >OK</button>';
> +    '>OK</button>';
>  var cancel_button_html =
>      '<button type="button" name="field.actions.cancel" ' +
> -    'class="lazr-neg lazr-btn" >Cancel</button>';
> +    '>Cancel</button>';

Thank you for removing the ambiguous and tiny icons. "Ok" is a poor
action term that Launchpad and other UI guidelines avoid. "Save" or
"Change" are better generic terms. If we have a separate action/link to
remove/clear duplicate, then this action would be named "Save
Duplicate".

> @@ -82,12 +85,250 @@
>          update_dupe_link.on('click', function(e) {
>              // Only go ahead if we have received the form content by the
>              // time the user clicks:
> -            if (that.duplicate_form_overlay) {
> +            if (that.duplicate_form) {
>                  e.halt();
> -                that.duplicate_form_overlay.show();
> +                that.duplicate_form.show();
>                  Y.DOM.byId('field.duplicateof').focus();
>              }
>          });
> +        // We the due form overlay is hidden, we need to reset the form.

Maybe s/We/When/;


> +    // Template for rendering the bug details.
> +    _bug_details_template: function() {
> +        return [
> +        '<div id="client-listing" style="max-width: 75em;">',

This looks too large. 60 is the max for English and that is what we
use else-where in Lp. Why does this need an exception?


+    // Template for the bug confirmation form.
+    _bug_confirmation_form_template: function() {
+        return [
+            '<div class="confirmation-node yui3-lazr-formoverlay-form" ',
+            'style="margin-top: 5px;">',

This is an odd number that does not fit with proportions. Our fonts our
12px, we assume 6px or 3px.

+            '{{> bug_details}}',
+            '<div class="yui3-lazr-formoverlay-errors"></div>',
+            '<div class="extra-form-buttons">',
+            '  <button class="yes_button" type="button">Select bug</button>',
+            '  <button class="no_button" type="button">Search again</button>',
+            '  <button class="cancel_button" type="button">Cancel</button>',
+            '</div>',
+            '</div>'].join('');
+    },

Lp button style is To capitalise each word in a button exceptfor prepositions or conjunctions.
-- 
https://code.launchpad.net/~wallyworld/launchpad/private-dupe-bug-warning2-943497/+merge/116793
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.