Code review comment for lp:~edwin-grubbs/launchpad/bug-513260-registry-js-module-names

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Summary
-------

Fixed conflicts between YUI module names, namespaces, and filenames.

Implementation details
----------------------

The DragScrollEventHandler was declared as a global instead of
in a YUI module, since it doesn't any YUI code. For consistency and
to avoid polluting the global namespace, I moved it into a YUI module.
This involved indenting all the code, so I have also provided a separate
diff at the bottom which shows the non-whitespace changes to the file.
    lib/canonical/launchpad/javascript/lp/dragscroll.js

The milestonetable.js file was being loaded by the productseries-index.pt
and distroseriesindex.pt files directly instead of the base layout. This
is bad since it prevents the js files from being combined.
    lib/lp/app/templates/base-layout-macros.pt
    lib/lp/registry/templates/distroseries-index.pt
    lib/lp/registry/templates/productseries-index.pt

Just renaming modules, namespaces, or files. The yui directory was
also moved from icing/yui/current/build to icing/yui a while ago, and
these tests had not been run since then, so I fixed that also.
    lib/canonical/launchpad/javascript/lp/mapping.js
    lib/canonical/launchpad/javascript/lp/picker.js
    lib/canonical/launchpad/javascript/registry/milestoneoverlay.js
    lib/canonical/launchpad/javascript/registry/milestonetable.js
    lib/canonical/launchpad/javascript/registry/tests/milestone_table.html
    lib/canonical/launchpad/javascript/registry/tests/test_milestone_table.js
    lib/canonical/launchpad/javascript/registry/tests/timeline-iframe.html
    lib/canonical/launchpad/javascript/registry/tests/timeline.html
    lib/canonical/launchpad/javascript/registry/tests/timeline.js
    lib/lp/registry/browser/__init__.py
    lib/lp/registry/browser/tests/productrelease-views.txt
    lib/lp/registry/browser/tests/productseries-views.txt
    lib/lp/registry/templates/object-timeline-graph.pt
    lib/lp/registry/templates/productrelease-add-from-series.pt

Tests
-----

./bin/test -vv --layer RegistryWindmillLayer
file:///./lib/canonical/launchpad/javascript/registry/tests/milestone_table.html
file:///./lib/canonical/launchpad/javascript/registry/tests/timeline.html

Demo and Q/A
------------

* Open https://launchpad.dev/firefox
    * Ensure that timeline graph still shows up and that it can be
      dragged if you zoom in until it doesn't fit.
* Open https://launchpad.dev/firefox/trunk
    * The "Create milestone" link should bring up a form overlay,
      and submitting the form should cause the milestone to appear
      in the table with a green flash.
* Open https://launchpad.dev/ubuntu/warty
    * Same test as for the firefox/trunk product series.
* Open https://launchpad.dev/people/+me/+editlocation
    * The map should appear and it should be possible to edit the
      location.
* Open https://bugs.launchpad.dev/bugs/3
    * The "Subscribe someone else" link should bring up a working picker.

Dragscroll diff without whitespace
----------------------------------

=== modified file 'lib/canonical/launchpad/javascript/lp/dragscroll.js'
--- lib/canonical/launchpad/javascript/lp/dragscroll.js 2009-10-08 17:13:13 +0000
+++ lib/canonical/launchpad/javascript/lp/dragscroll.js 2010-02-11 20:11:36 +0000
@@ -1,4 +1,10 @@
-/*
+/* Copyright 2009 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * A milestone form overlay that can create a milestone within any page.
+ *
+ * @module Y.lp.dragscroll
+ *
  * Based on dragscroll script by Nicolas Mendoza <email address hidden>.
  * http://people.opera.com/nicolasm/userjs/dragscroll
  */
@@ -9,13 +15,16 @@
  * @class DragScrollEventHandler
  * @constructor
  */
-DragScrollEventHandler = function() {
+YUI.add('lp.dragscroll', function(Y) {
+ var module = Y.namespace('lp.dragscroll');
+
+ module.DragScrollEventHandler = function() {
     this.dragging = false;
     this.last_position = null;
     this.event_listeners = [];
-}
+ };

-DragScrollEventHandler.prototype = {
+ module.DragScrollEventHandler.prototype = {
     /**
      * Add the event handlers and change the cursor to indicate
      * that drag scrolling is active.
@@ -48,7 +57,7 @@
         // `this` to be the `DragScrollEventHandler` object.
         var self = this;
         var event_listener = function(e) {
- action.call(self, e)
+ action.call(self, e);
         };
         var event_args = [event_type, event_listener, false];
         this.event_listeners.push(event_args);
@@ -90,7 +99,7 @@
      * @method _startDragScroll
      */
     _startDragScroll: function(e) {
- if (e.button == 0) {
+ if (e.button === 0) {
             this.dragging = true;
             this.last_position = e;
             this._setGrabbingCursor();
@@ -131,4 +140,5 @@
             e.stopPropagation();
         }
     }
-};
+ };
+}, "0.1", {"requires": []});

« Back to merge proposal