Code review comment for lp:~wallyworld/launchpad/private-dupe-bug-warning2-943497

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.