launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/halt-when-done-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #961923 in Launchpad itself: "ChoiceSource doesn't work in IE"
  https://bugs.launchpad.net/launchpad/+bug/961923

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/halt-when-done-2/+merge/105859

Pre-implementation: wallyworld, wgrant

The ChoiseSource picker does not work in msie. Some preliminary work
to make it use halt() fixed some issues to setup the page. the onclick
event always returns early though in msie.

--------------------------------------------------------------------

RULES

    * YUI docs state that e.button is for mouseup events, not click event.
      http://yuilibrary.com/yui/docs/api/classes/DOMEventFacade.html#property_button
      http://yuilibrary.com/yui/docs/event/#mouse-event-properties
      * Remove the mouse button check because there is no use-case
        to support it.
    * Remove the secondary guards that block the setup of ChoiceSource.
      in many templates and modules.

    ADDENDUM
    * wgrant suggests an alternate combination of properties to create
      the overlay shadow in msie.
      https://bugs.launchpad.net/launchpad/+bug/998229


QA

    http://people.canonical.com/~curtis/msie-overlay-shadow.png
    demonstrated wgrant's fix for the box shadow.

    * Visit https://bugs.qastaging.launchpad.net/gdp/+bug/420173
    * Verify you can use the affects me.
    * Verify it has a shadow that fills all four corners.
    * Verify you can set the status.
    * Verify you can set the information type.
    * Visit https://code.qastaging.launchpad.net/~sinzui/gdp/packaging/+merge/94307
    * Verify you can change the merge proposal status.
    * Visit https://blueprints.qastaging.launchpad.net/gdp/+spec/gdplaunchpad
    * Verify you can change the direct approved.


LINT

    lib/lp/app/javascript/choice.js
    lib/lp/app/javascript/choiceedit/choiceedit.js
    lib/lp/app/javascript/choiceedit/tests/test_choiceedit.js
    lib/lp/app/javascript/overlay/assets/pretty-overlay-core.css
    lib/lp/bugs/javascript/bugtask_index.js
    lib/lp/code/templates/branchmergeproposal-index.pt


TEST

    ./bin/test -vvc --layer=YUITest -t 'lp/(app|bugs|code)' lp


IMPLEMENTATION

Updated the CSS to draw a shadow in all four cornders in msie.
    lib/lp/app/javascript/overlay/assets/pretty-overlay-core.css

Removed the left button check from the onclick handler per YUI
documentation.
    lib/lp/app/javascript/choiceedit/choiceedit.js
    lib/lp/app/javascript/choiceedit/tests/test_choiceedit.js

Enabled the boolean choice source on the blueprint page.
Enabled the enum choise source on the bugtask and merge proposal pages.
    lib/lp/app/javascript/choice.js
    lib/lp/bugs/javascript/bugtask_index.js
    lib/lp/code/templates/branchmergeproposal-index.pt
-- 
https://code.launchpad.net/~sinzui/launchpad/halt-when-done-2/+merge/105859
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/halt-when-done-2 into lp:launchpad.

=== modified file 'lib/lp/app/javascript/choice.js'
--- lib/lp/app/javascript/choice.js	2011-04-19 18:03:50 +0000
+++ lib/lp/app/javascript/choice.js	2012-05-15 17:07:48 +0000
@@ -22,11 +22,6 @@
 }
 
 namespace.addBinaryChoice = function(config, resource_uri, attribute) {
-
-  if (Y.UA.ie) {
-    return;
-  }
-
   var widget = new Y.ChoiceSource(config);
   widget.plug({
     fn: Y.lp.client.plugins.PATCHPlugin,

=== modified file 'lib/lp/app/javascript/choiceedit/choiceedit.js'
--- lib/lp/app/javascript/choiceedit/choiceedit.js	2012-03-15 05:40:30 +0000
+++ lib/lp/app/javascript/choiceedit/choiceedit.js	2012-05-15 17:07:48 +0000
@@ -242,12 +242,6 @@
      * @private
      */
     onClick: function(e) {
-
-        // Only continue if the down button is the left one.
-        if (e.button !== LEFT_MOUSE_BUTTON) {
-            return;
-        }
-
         this._choice_list = new Y.ChoiceList({
             value:          this.get("value"),
             title:          this.get("title"),

=== modified file 'lib/lp/app/javascript/choiceedit/tests/test_choiceedit.js'
--- lib/lp/app/javascript/choiceedit/tests/test_choiceedit.js	2012-04-06 17:28:25 +0000
+++ lib/lp/app/javascript/choiceedit/tests/test_choiceedit.js	2012-05-15 17:07:48 +0000
@@ -117,13 +117,6 @@
               "ChoiceList HTML is not being added to the page");
         },
 
-        test_right_clicking_doesnt_create_choicelist: function() {
-            simulate(this.choice_edit.get('boundingBox'),
-                     '.value', 'click', { button: 2 });
-            Assert.isNull(Y.one(document).one(".yui3-ichoicelist"),
-              "ChoiceList created when the right mouse button was clicked");
-        },
-
         test_choicelist_has_correct_values: function() {
             simulate(this.choice_edit.get('boundingBox'), '.value', 'click');
             var that = this;

=== modified file 'lib/lp/app/javascript/overlay/assets/pretty-overlay-core.css'
--- lib/lp/app/javascript/overlay/assets/pretty-overlay-core.css	2012-05-12 01:55:23 +0000
+++ lib/lp/app/javascript/overlay/assets/pretty-overlay-core.css	2012-05-15 17:07:48 +0000
@@ -22,7 +22,7 @@
     text-align: left;
     border-radius: 5px;
     -webkit-box-shadow: 0px 0px 20px 10px #aaa;
-    -ms-filter: "progid:DXImageTransform.Microsoft.Shadow(Strength=15, Direction=135, Color='#aaaaaa'), progid:DXImageTransform.Microsoft.Shadow(Strength=15, Direction=315, Color='#aaaaaa')";
+    -ms-filter: "progid:DXImageTransform.Microsoft.Shadow(Strength=15, Direction=0, Color='#aaaaaa'), progid:DXImageTransform.Microsoft.Shadow(Strength=15, Direction=90, Color='#aaaaaa'), progid:DXImageTransform.Microsoft.Shadow(Strength=15, Direction=180, Color='#aaaaaa'), progid:DXImageTransform.Microsoft.Shadow(Strength=15, Direction=270, Color='#aaaaaa')";
     box-shadow: 0px 0px 20px 10px #aaa;
     }
 

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2012-05-11 20:34:22 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2012-05-15 17:07:48 +0000
@@ -889,10 +889,6 @@
     /* ChoiceSource doesn't seem to function in IE at present, breaking
      * all of this AJAX except the assignee and product pickers.
      */
-    if (Y.UA.ie) {
-        return;
-    }
-
     var tr = Y.one('#' + conf.row_id);
     var bugtarget_content = Y.one('#bugtarget-picker-' + conf.row_id);
     var status_content = tr.one('.status-content');
@@ -1194,9 +1190,6 @@
  */
 namespace.setup_me_too = function(user_is_affected, others_affected_count) {
     /* ChoiceSource is broken in IE8, probably IE9. */
-    if (Y.UA.ie) {
-        return;
-    }
     var me_too_content = Y.one('#affectsmetoo');
     var me_too_edit = new MeTooChoiceSource({
         contentBox: me_too_content, value: user_is_affected,

=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt	2012-02-01 15:31:32 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt	2012-05-15 17:07:48 +0000
@@ -217,11 +217,6 @@
         if (logged_in) {
             var comment = new Y.lp.app.comment.CodeReviewComment();
             comment.render();
-
-            if(Y.UA.ie) {
-                return;
-            }
-
             Y.lp.code.branchmergeproposal.status.connect_status(conf);
         }
         Y.lp.code.branchmergeproposal.reviewcomment.connect_links();