Merge into devel : private-dupe-bug-warning2-943497 : Code : Launchpad itself

Status:

Merged

Approved by:

Curtis Hovey on 2012-07-27

Approved revision:

no longer in the source branch.

Merged at revision:

15719

Proposed branch:

lp://staging/~wallyworld/launchpad/private-dupe-bug-warning2-943497

Merge into:

lp://staging/launchpad

Diff against target:

809 lines (+434/-98)

7 files modified

lib/canonical/launchpad/icing/css/forms.css (+1/-2)
lib/canonical/launchpad/icing/css/modifiers.css (+8/-0)
lib/lp/app/javascript/formoverlay/assets/formoverlay-core.css (+10/-0)
lib/lp/app/javascript/formoverlay/formoverlay.js (+14/-11)
lib/lp/bugs/javascript/duplicates.js (+278/-29)
lib/lp/bugs/javascript/tests/test_duplicates.html (+1/-0)
lib/lp/bugs/javascript/tests/test_duplicates.js (+122/-56)

To merge this branch:

bzr merge lp://staging/~wallyworld/launchpad/private-dupe-bug-warning2-943497

Related bugs:

Bug #190635: Cannot tell that you chose the right bug when marking as a duplicate	Low	Fix Released
Bug #767084: "Mark as duplicate" popup gives an unhelpful error when attempted to dupe against a dupe	Low	Fix Released
Bug #943497: warn when you are going to mark a public bug as a duplicate of a private one	High	Fix Released
Bug #1023425: Duplicate error "not a valid bug number or nickname" when nicknames are defunct	Low	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Curtis Hovey (community)	code	2012-07-26	Approve on 2012-07-26
Review via email: mp+116793@code.staging.launchpad.net

Commit message

Initial work to improve the bug duplicate form overlay to allow the selected bug to be previewed prior to submission.

Description of the change

== Implementation ==

This branch fixes a number of issues with the mark bug as duplicate form. See the linked bugs. The work is currently only for the bug selection widget used when marking bugs as duplicates. It would need to be generalised to be useful when selecting bugs to link to branches etc.

Not quite everything is implemented in this branch but it's big enough to be put up for review. The bit that is missing is marked with a TODO.

When a bug number is entered, a web service API call is made to get the json data for the bug. This data is presented to the user to allow then to confirm the bug is what they want. The TODO is because not all necessary data is returned and is currently hard coded. A new service API needs to be added to provided the required data.

Some css changes were necessary. Here some of the "non obvious" ones...

Needed to make spacing around form overlay form table correct due to css rule ordering :-

13 table.form, table.extra-options {
...
16 - margin: 1em 0;
17 + margin: 1em 0 inherit inherit;

Needed to un-squash form buttons when they are not little lazr icons.

48 +.yui3-lazr-formoverlay-form div.yui3-lazr-formoverlay-actions button {
49 + margin-right: 0.7em;
50 + }
51

Expand out the error div on the form overlay when there is an error so it is not all squashed up when the form is narrow.

58 +.yui3-lazr-formoverlay-form .yui3-lazr-formoverlay-errors:not(:empty) {
59 + min-width: 250px;
60 + text-align: center;
61 + }
62 +

== Demo and QA ==

Here's an early video of it in operation.

http://people.canonical.com/~ianb/bug-dup.ogv

This video has a number of minor flaws (eg uncentred form, text alignment). These are addressed in this branch.

== Tests ==

Add a number of new yui tests and update existing ones.

== Lint ==

Linting changed files:
  lib/canonical/launchpad/icing/css/forms.css
  lib/canonical/launchpad/icing/css/modifiers.css
  lib/lp/app/javascript/formoverlay/formoverlay.js
  lib/lp/app/javascript/formoverlay/assets/formoverlay-core.css
  lib/lp/bugs/javascript/duplicates.js
  lib/lp/bugs/javascript/tests/test_duplicates.html
  lib/lp/bugs/javascript/tests/test_duplicates.js

Clean except for some css noise.

Revision history for this message

Curtis Hovey (sinzui) wrote on 2012-07-26:

#

Download full text (4.0 KiB)

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. Wh...

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.

review: Approve (code)

Launchpad itself

Merge lp://staging/~wallyworld/launchpad/private-dupe-bug-warning2-943497 into lp://staging/launchpad

Commit message

Description of the change

Preview Diff

Subscribers