launchpad-reviewers team mailing list archive

[Deryck Hodge <deryck.hodge@xxxxxxxxxxxxx>]

Deryck Hodge has proposed merging lp:~deryck/launchpad/base-config-widget into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~deryck/launchpad/base-config-widget/+merge/81302

I'm working on adding a widget for hiding and showing fields in our new bug lists as part of the CustomBugListings feature.  This widget is opened by clicking a settings cog icon.  See the prototype html here to see it in action:

http://people.canonical.com/~deryck/new-buglistings/new-buglistings.html

(Note that the prototype is out of sync with real code now, but you can at least see how the cog icon should look and behave.)

As a first slice, I created a BaseConfig widget in this branch.  Any widget that wants to be fired from a cog icon can extend this new BaseConfig widget.  The widget adds the html and classes needed to provide the cog icon as well as methods for any subclass to hook into that will allow additional UI rendering and click handling.  The branch also adds the png image and CSS to make sure the sprite file is regenerated correctly.

This widget is unused in LP, though I have a second branch going to extend it for my ToggleDisplayConfig widget.  I'll land the later widget separately and then do a final branch to integrate this into LP.

We're lint free except for the always noisy and seldom usefulC SS lint warnings (about lines being too long across the file).

Test coverage can be run by pointing your browser at:
$BRANCH/lib/lp/app/javascript/tests/test_config.html

-- 
https://code.launchpad.net/~deryck/launchpad/base-config-widget/+merge/81302
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/base-config-widget into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/sprite.css.in'
--- lib/canonical/launchpad/icing/sprite.css.in	2011-08-04 13:56:55 +0000
+++ lib/canonical/launchpad/icing/sprite.css.in	2011-11-04 17:14:29 +0000
@@ -17,6 +17,10 @@
     background-image: url(/@@/remove.png); /* sprite-ref: icon-sprites */
     background-repeat: no-repeat;
     }
+.config {
+    background-image: url(/@@/config.png); /* sprite-ref: icon-sprites */
+    background-repeat: no-repeat;
+    }
 .info {
     background-image: url(/@@/info.png); /* sprite-ref: icon-sprites */
     background-repeat: no-repeat;

=== added file 'lib/canonical/launchpad/images/config.png'
Binary files lib/canonical/launchpad/images/config.png	1970-01-01 00:00:00 +0000 and lib/canonical/launchpad/images/config.png	2011-11-04 17:14:29 +0000 differ
=== added file 'lib/lp/app/javascript/config.js'
--- lib/lp/app/javascript/config.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/config.js	2011-11-04 17:14:29 +0000
@@ -0,0 +1,96 @@
+/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
+
+YUI().add('lp.config', function(Y) {
+    /**
+     * The config module provides objects for managing the config
+     * or settings of a web page or widget.
+     *
+     * Widgets that want to be accessed from a settings/config
+     * icon should extend BaseConfig and provide a callback that
+     * will run when the icon is clicked.
+     *
+     * @module lp.config
+     */
+
+    // Constants
+    var CONTENT_BOX = 'contentBox',
+        EMPTY_FN = function() {};
+
+    /**
+     * BaseConfig is the base object that every XXXConfig object
+     * should extend.
+     *
+     * @class BaseConfig
+     * @extends Widget
+     * @constructor
+     */
+    function BaseConfig() {
+        BaseConfig.superclass.constructor.apply(this, arguments);
+    }
+
+    BaseConfig.NAME = 'baseconfig';
+
+    BaseConfig.ATTRS = {
+        /**
+         * A reference to the anchor element created during renderUI.
+         *
+         * @attribute anchor
+         * @type Y.Node
+         * @default null
+         */
+        anchor: {
+            value: null
+        }
+    };
+
+    Y.extend(BaseConfig, Y.Widget, {
+
+        /**
+         * Hook for subclasses to do something when the settings
+         * icon is clicked.
+         */
+        _handleClick: EMPTY_FN,
+
+        /**
+         * Hook for subclasses to do work after renderUI.
+         */
+        _extraRenderUI: EMPTY_FN,
+
+        /**
+         * Create the anchor element that will display the settings icon.
+         *
+         * @method renderUI
+         */
+        renderUI: function() {
+            var anchor = Y.Node.create(
+                '<a></a>').addClass('sprite').addClass('config');
+            this.set('anchor', anchor);
+            var content = this.get(CONTENT_BOX);
+            content.append(anchor);
+            this._extraRenderUI();
+        },
+
+        /**
+         * Wire up the anchor element to _handleClick.
+         *
+         * Objects that extend BaseConfig should create their own
+         * _handleClick method.
+         *
+         * @method bindUI
+         */
+        bindUI: function() {
+            // Do some work here to set up click handlers.
+            // Add the a element to ATTRS.
+            var anchor = this.get('anchor');
+            var that = this;
+            anchor.on('click', function(e) {
+                that._handleClick(e);
+            });
+        }
+
+    });
+
+    var config = Y.namespace('lp.config');
+    config.BaseConfig = BaseConfig;
+
+}, '0.1', {'requires': ['widget']});

=== added file 'lib/lp/app/javascript/tests/test_config.html'
--- lib/lp/app/javascript/tests/test_config.html	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/test_config.html	2011-11-04 17:14:29 +0000
@@ -0,0 +1,27 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
+  "http://www.w3.org/TR/html4/strict.dtd";>
+<html>
+  <head>
+  <title>Tests for Config Widgets</title>
+
+  <!-- YUI and test setup -->
+  <script type="text/javascript"
+          src="../../../../canonical/launchpad/icing/yui/yui/yui.js">
+  </script>
+  <link rel="stylesheet" href="../testing/test.css" />
+  <script type="text/javascript"
+          src="../testing/testrunner.js"></script>
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../config.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_config.js"></script>
+
+</head>
+<body class="yui3-skin-sam">
+    <ul id="suites">
+        <li>lp.baseconfig.test</li>
+    </ul>
+</body>
+</html>

=== added file 'lib/lp/app/javascript/tests/test_config.js'
--- lib/lp/app/javascript/tests/test_config.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/tests/test_config.js	2011-11-04 17:14:29 +0000
@@ -0,0 +1,98 @@
+/* Copyright (c) 2011, Canonical Ltd. All rights reserved. */
+
+YUI.add('lp.baseconfig.test', function(Y) {
+
+var baseconfig_test = Y.namespace('lp.baseconfig.test');
+
+var suite = new Y.Test.Suite('BaseConfig Tests');
+
+var Assert = Y.Assert;
+
+suite.add(new Y.Test.Case({
+
+    name: 'baseconfig_widget_tests',
+
+    tearDown: function() {
+        if (Y.Lang.isValue(this.baseconfig)) {
+            this.baseconfig.destroy();
+        }
+    },
+
+    _makeSrcNode: function(id) {
+        var src_node = Y.Node.create('<div></div>').set('id', id);
+        Y.one('body').appendChild(src_node);
+    },
+
+    test_base_config_render: function() {
+        // The div rendered should have sprite and config
+        // class names added to it.
+        this._makeSrcNode();
+        this.baseconfig = new Y.lp.config.BaseConfig({
+            srcNode: Y.one('#test-div')
+        });
+        this.baseconfig.render();
+        var config_a = Y.one('.yui3-baseconfig a');
+        Assert.isTrue(config_a.hasClass('sprite'));
+        Assert.isTrue(config_a.hasClass('config'));
+    },
+
+    test_base_config_anchor_attribute: function() {
+        // BaseConfig keeps a reference to the DOM node it created.
+        this._makeSrcNode();
+        this.baseconfig = new Y.lp.config.BaseConfig({
+            srcNode: Y.one('#test-div')
+        });
+        // anchor should be null before render.
+        Assert.isNull(this.baseconfig.get('anchor'));
+        // After render, "anchor" attribute should match the node via DOM.
+        this.baseconfig.render();
+        var config_via_dom = Y.one('.yui3-baseconfig a');
+        Assert.areEqual(config_via_dom, this.baseconfig.get('anchor'));
+    },
+
+    test_base_config_click_callback: function() {
+        // _handleClick should be called when the settings
+        // icon is clicked.
+        this._makeSrcNode();
+        this.baseconfig = new Y.lp.config.BaseConfig({
+            srcNode: Y.one('#test-div')
+        });
+        // _handleClick already exists but does nothing.
+        Assert.areSame(this.baseconfig._handleClick(), undefined);
+        var click_handled = false;
+        this.baseconfig._handleClick = function(e) {
+            click_handled = true;
+        };
+        this.baseconfig.render();
+        Y.one('.config').simulate('click');
+        Assert.isTrue(click_handled);
+    },
+
+    test_base_config_extra_render_ui: function() {
+        // BaseConfig provides a hook for subclasses to do
+        // additional renderUI work.
+        this._makeSrcNode();
+        this.baseconfig = new Y.lp.config.BaseConfig({
+            srcNode: Y.one('#test-div')
+        });
+        // _extraRenderUI already exists but does nothing.
+        Assert.areSame(this.baseconfig._extraRenderUI(), undefined);
+        var href = 'http://example.com/';
+        var html = 'This is some sample text to add.';
+        var that = this;
+        this.baseconfig._extraRenderUI = function() {
+            var anchor = that.baseconfig.get('anchor');
+            anchor.set('href', href);
+            anchor.set('innerHTML', html);
+        };
+        this.baseconfig.render();
+        var anchor = this.baseconfig.get('anchor');
+        Assert.areEqual(href, anchor.get('href'));
+        Assert.areEqual(html, anchor.get('innerHTML'));
+    }
+
+}));
+
+baseconfig_test.suite = suite;
+
+}, '0.1', {'requires': ['test', 'node-event-simulate', 'lp.config']});