Merge lp:~wallyworld/lazr-js/picker-entry-extra-title-links into lp:lazr-js

[Ian Booth]

[r=sinzui][bug=791053] Enhance the picker to display an alternate title entry and optionally linkify the title/alt titles.

For the disclosure feature work, enhancements are required to the picker - extra information needs to be rendered so that the user can be confident that they are choosing the correct entry.

This branch provides the infrastructure to allow picker implementations to:
1. Render an alternate title for each picker entry - rendered in parentheses after the main title
2. Optionally provide hrefs for title/alt title which cause these to be rendered as anchors and the links are opened in new windows.
3. Optionally render badges (small images) to the right of the picker entry title.

Building the tarball to test this branch revealed a bug introduced to build.py. A change was made to allow the combo build to be invoked from the source directory rather than the egg. This broke Launchpad's jsbuild so a fix was done to cater for both scenarios.

== Implementation ==

Modify the picker rendering javascript. The title and description rendering is broken out to separate functions. The description is not rendered any differently but may be for subsequent work.

There's one lint error - I'm not sure exactly what I need to change to fix it.

== Demo ==

See screenshot : http://people.canonical.com/~ianb/person-picker-links.png
This example shows just the alternate titles having links.

== Tests ==

Add new tests to picker/tests/picker.js

== Lint ==

jslint: 2 files to lint.
jslint: No problem found in '/home/ian/projects/lazrjs-branches/devel/src-js/lazrjs/picker/picker.js'.

jslint: Lint found in '/home/ian/projects/lazrjs-branches/devel/src-js/lazrjs/picker/tests/picker.js':
Line 158 character 52: Script URL.
            Assert.areEqual(link_node.get('href'), 'javascript:void(0)');

[lp:~wallyworld/lazr-js/picker-entry-extra-title-links]

211. By Ian Booth on 2011-06-01

Fix escaping issue

212. By Ian Booth on 2011-06-01

Fix typo

213. By Ian Booth on 2011-06-02

Add support for picker item badges

214. By Ian Booth on 2011-06-02

Add results badge style

215. By Ian Booth on 2011-06-02

Fix build and remove styling from badges

216. By Ian Booth on 2011-06-02

Fix tests

217. By Ian Booth on 2011-06-02

Add css class to badge images

218. By Ian Booth on 2011-06-03

Tidy up

[Curtis Hovey (sinzui)]

Hi Ian.

Thank you for providing this branch. I see a few trivial issues I would like you to fix. I also have some reservation about the action link, but I think user testing is more important than my own musings.

> === modified file 'src-js/lazrjs/picker/picker.js'
> --- src-js/lazrjs/picker/picker.js 2011-01-14 15:50:06 +0000
> +++ src-js/lazrjs/picker/picker.js 2011-06-03 02:06:33 +0000
> @@ -317,6 +317,93 @@
> },
>
> /**
> + * Return a node containing the specified text. If a href is provided,
> + * then the text will be linkified with with the given css class. The
> + * link will open in a new window (but the browser can be configured to
> + * open a new tab instead if the user so wishes).
> + * @param text the text to render
> + * @param href the URL of the new window
> + * @param css the style to use when rendering the link
> + */
> + _text_or_link: function(text, href, css) {
> + var result;
> + if (href) {
> + result=Y.Node.create(

whitespace around operators.

+ "<a class='"+css+"' href=javascript:void(0)></a>");

whitespace around operators.

> + result.set('text', text);
> + Y.on('click', function(e) {
> + e.halt();
> + window.open(href);
> + }, result);
> + } else {
> + result = document.createTextNode(text);
> + }
> + return result;
> + },
> +
> + /**
> + * Render a node containing the title part of the picker entry.
> + * The title will consist of some main text with some optional alternate
> + * text which will be rendered in parentheses after the main text. The
> + * title/alt_title text may separately be turned into a link with user
> + * specified URLs.
> + * @param data a json data object with the details to render
> + */

I worry that this presupposes every user of lazrjs using display names and
Ids like Lp does. I think we may need to revisit this. I would not be
surprised if other lazr stakeholders would want this to follow the Web UI
guidelines; an action link:
    action >
or an action button
    [action button]

These are also the same guidelines that define the green we use of actions
that do not leave the current page/context.

> + _renderTitleUI: function(data) {
> + var li_title = Y.Node.create(
> + '<span></span>').addClass(C_RESULT_TITLE);
> + var title = this._text_or_link(
> + data.title, data.title_link, data.link_css);
> + li_title.appendChild(title);
> + if (data.alt_title) {
> + var alt_title = this._text_or_link(
> + data.alt_title, data.alt_title_link, data.link_css);
> + li_title.appendChild('&nbsp(')

I think this is an invalid named entity. Entities are terminated with a
semi-colon.

> + .appendChild(alt_title)
> + .appendChild(')');
> + }
> + return li_title;
> + },

...

> === modified file 'src-js/lazrjs/picker/tests/picker.js'
> --- src-js/lazrjs/picker/tests/picker.js 2011-01-14 17:13:05 +0000
> +++ src-js/lazrjs/picker/tests/picker.js 2011-06-03 ...

review: Approve (code)

[Ian Booth (wallyworld)]

Thanks for reviewing.

>
> whitespace around operators.
>
> + "<a class='"+css+"' href=javascript:void(0)></a>");
>
> whitespace around operators.
>

Doh. I should know that by now. Sorry.

>> +
>> + /**
>> + * Render a node containing the title part of the picker entry.
>> + * The title will consist of some main text with some optional alternate
>> + * text which will be rendered in parentheses after the main text. The
>> + * title/alt_title text may separately be turned into a link with user
>> + * specified URLs.
>> + * @param data a json data object with the details to render
>> + */
>
> I worry that this presupposes every user of lazrjs using display names and
> Ids like Lp does. I think we may need to revisit this. I would not be
> surprised if other lazr stakeholders would want this to follow the Web UI
> guidelines; an action link:
> action >
> or an action button
> [action button]
>

I've made it generic - there's no pre assumption about the intent of the
title or alt title. It's treated as simply text. The css style to use
for the links is also user specified. But yes, there's no allowance for
rendering anything other that a link which opens in a new window. We can
always revist and enhance this as required. I did just enough for the
current use case and anything else might have ended up being YAGNI.

> These are also the same guidelines that define the green we use of actions
> that do not leave the current page/context.
>

The css style which colours the rendered link green is passed in from
the lp side. lazr-js has no knowledge of what the user specified style
means.

>> + var alt_title = this._text_or_link(
>> + data.alt_title, data.alt_title_link, data.link_css);
>> + li_title.appendChild('&nbsp(')
>
> I think this is an invalid named entity. Entities are terminated with a
> semi-colon.
>
>> + .appendChild(alt_title)
>> + .appendChild(')');
>> + }
>> + return li_title;
>> + },
>
> ...

You sure about the above comment? The code is just
li_title.appendChild(xxx).appendChild(xxx).appendChild(xxx)
split over a few lines to avoid exceeding 78 chars.

[lp:~wallyworld/lazr-js/picker-entry-extra-title-links]

219. By Ian Booth on 2011-06-03

Lint fixes

[Ian Booth (wallyworld)]

Hi Curtis

I'm really sorry - I totally misinterpreted your comment below.
I've added the missing ; in the correct place. Don't know what I was
smoking there.

>
>>> + var alt_title = this._text_or_link(
>>> + data.alt_title, data.alt_title_link, data.link_css);
>>> + li_title.appendChild('&nbsp(')
>>
>> I think this is an invalid named entity. Entities are terminated with a
>> semi-colon.
>>
>>> + .appendChild(alt_title)
>>> + .appendChild(')');
>>> + }
>>> + return li_title;
>>> + },
>>
>> ...
>
>
> You sure about the above comment? The code is just
> li_title.appendChild(xxx).appendChild(xxx).appendChild(xxx)
> split over a few lines to avoid exceeding 78 chars.
>
>

=== modified file 'src-js/lazrjs/picker/picker.js'
--- src-js/lazrjs/picker/picker.js	2011-01-14 15:50:06 +0000
+++ src-js/lazrjs/picker/picker.js	2011-06-03 06:39:38 +0000
@@ -317,6 +317,93 @@
     },
 
     /**
+     * Return a node containing the specified text. If a href is provided,
+     * then the text will be linkified with with the given css class. The
+     * link will open in a new window (but the browser can be configured to
+     * open a new tab instead if the user so wishes).
+     * @param text the text to render
+     * @param href the URL of the new window
+     * @param css the style to use when rendering the link
+     */
+    _text_or_link: function(text, href, css) {
+        var result;
+        if (href) {
+            result=Y.Node.create(
+                "<a class = '" + css + "' href=javascript:void(0)></a>");
+            result.set('text', text);
+            Y.on('click', function(e) {
+                e.halt();
+                window.open(href);
+            }, result);
+        } else {
+            result = document.createTextNode(text);
+        }
+        return result;
+    },
+
+    /**
+     * Render a node containing the title part of the picker entry.
+     * The title will consist of some main text with some optional alternate
+     * text which will be rendered in parentheses after the main text. The
+     * title/alt_title text may separately be turned into a link with user
+     * specified URLs.
+     * @param data a json data object with the details to render
+     */
+    _renderTitleUI: function(data) {
+        var li_title = Y.Node.create(
+            '<span></span>').addClass(C_RESULT_TITLE);
+        var title = this._text_or_link(
+            data.title, data.title_link, data.link_css);
+        li_title.appendChild(title);
+        if (data.alt_title) {
+            var alt_title = this._text_or_link(
+                data.alt_title, data.alt_title_link, data.link_css);
+            li_title.appendChild('&nbsp(')
+                .appendChild(alt_title)
+                .appendChild(')');
+        }
+        return li_title;
+    },
+
+    /**
+     * Render a node containing the badges part of the picker entry.
+     * The badges are small images which are displayed next to the title. The
+     * display of badges is optional.
+     * @param data a json data object with the details to render
+     */
+    _renderBadgesUI: function(data) {
+        if (data.badges) {
+            var badges = Y.Node.create('<div></div>').addClass('badge');
+            Y.each(data.badges, function(badge_info) {
+                var badge_url = badge_info.url;
+                var badge_alt = badge_info.alt;
+                var badge = Y.Node.create('<img></img>')
+                    .addClass('badge')
+                    .set('src', badge_url)
+                    .set('alt', badge_alt);
+                badges.appendChild(badge);
+            });
+            return badges;
+        }
+        return null;
+    },
+
+    /**
+     * Render a node containing the description part of the picker entry.
+     * @param data a json data object with the details to render
+     */
+    _renderDescriptionUI: function(data) {
+        var li_desc = Y.Node.create(
+            '<div><br /></div>').addClass(C_RESULT_DESCRIPTION);
+        if (data.description) {
+            li_desc.replaceChild(
+                document.createTextNode(data.description),
+                li_desc.one('br'));
+        }
+        return li_desc;
+    },
+
+    /**
      * Update the UI based on the results attribute.
      *
      * @method _syncResultsUI
@@ -330,19 +417,12 @@
         this._results_box.set('innerHTML', '');
 
         Y.Array.each(results, function(data, i) {
+            // Sort out the badges div.
+            var li_badges = this._renderBadgesUI(data);
             // Sort out the title span.
-            var li_title = Y.Node.create(
-                '<span></span>').addClass(C_RESULT_TITLE);
-            li_title.appendChild(
-                document.createTextNode(data.title));
+            var li_title = this._renderTitleUI(data);
             // Sort out the description div.
-            var li_desc = Y.Node.create(
-                '<div><br /></div>').addClass(C_RESULT_DESCRIPTION);
-            if (data.description) {
-                li_desc.replaceChild(
-                    document.createTextNode(data.description),
-                    li_desc.one('br'));
-            }
+            var li_desc = this._renderDescriptionUI(data);
             // Put the list item together.
             var li = Y.Node.create('<li></li>').addClass(
                 i % 2 ? Y.lazr.ui.CSS_ODD : Y.lazr.ui.CSS_EVEN);
@@ -353,6 +433,8 @@
                 li.appendChild(
                     Y.Node.create('<img />').set('src', data.image));
             }
+            if (li_badges !== null)
+                li.appendChild(li_badges);
             li.appendChild(li_title);
             li.appendChild(li_desc);
             // Attach handlers.

=== modified file 'src-js/lazrjs/picker/tests/picker.js'
--- src-js/lazrjs/picker/tests/picker.js	2011-01-14 17:13:05 +0000
+++ src-js/lazrjs/picker/tests/picker.js	2011-06-03 06:39:38 +0000
@@ -109,6 +109,90 @@
             'Unexpected description value.');
     },
 
+    test_alternate_title_text: function () {
+        this.picker.render();
+        this.picker.set('results', [
+            {
+                css: 'yui3-blah-blue',
+                value: 'jschmo',
+                title: 'Joe Schmo',
+                description: '[e-mail]',
+                alt_title: 'Another Joe'
+            }
+        ]);
+        var bb = this.picker.get('boundingBox');
+        var li = bb.one('.yui3-picker-results li');
+        var title_el = li.one('.yui3-picker-result-title');
+        Assert.isNotNull(title_el, "Missing title element");
+        Assert.areEqual(
+            'Joe Schmo\u00a0(Another Joe)', title_el.get('text'),
+            'Unexpected title value.');
+    },
+
+    test_title_links: function () {
+        this.picker.render();
+        this.picker.set('results', [
+            {
+                css: 'yui3-blah-blue',
+                value: 'jschmo',
+                title: 'Joe Schmo',
+                description: '[e-mail]',
+                alt_title: 'Joe Again <foo></foo>',
+                title_link: 'http://somewhere.com',
+                alt_title_link: 'http://somewhereelse.com',
+                link_css: 'cool-style'
+            }
+        ]);
+
+        function check_link(picker, link_selector, title) {
+            var bb = picker.get('boundingBox');
+            var link_clicked = false;
+            var link_node = bb.one(link_selector);
+
+            Assert.areEqual(title, link_node.get('text'));
+            Assert.areEqual('javascript:void(0)', link_node.get('href'));
+
+            Y.on('click', function(e) {
+                link_clicked = true;
+            }, link_node);
+            simulate(bb, link_selector, 'click');
+            Assert.isTrue(link_clicked,
+                link_selector + ' link was not clicked');
+        }
+        check_link(
+            this.picker, '.cool-style:nth-child(1)', 'Joe Schmo');
+        check_link(
+            this.picker, '.cool-style:nth-child(2)', 'Joe Again <foo></foo>');
+    },
+
+    test_title_badges: function () {
+        this.picker.render();
+        var badge_info = [
+            {url: '../../lazr/assets/skins/sam/search.png', alt: 'alt 1'},
+            {url: '../../lazr/assets/skins/sam/spinner.png', alt: 'alt 2'}];
+        this.picker.set('results', [
+            {
+                badges: badge_info,
+                css: 'yui3-blah-blue',
+                value: 'jschmo',
+                title: 'Joe Schmo',
+                description: '[e-mail]'
+            }
+        ]);
+        var bb = this.picker.get('boundingBox');
+        var li = bb.one('.yui3-picker-results li');
+        for (var i=0; i<badge_info.length; i++) {
+            var img_node = li.one(
+                'div.badge img.badge:nth-child(' + (i + 1) + ')');
+            Assert.areEqual(
+                badge_info[i].url, img_node.getAttribute('src'),
+                'Unexpected badge url');
+            Assert.areEqual(
+                badge_info[i].alt, img_node.get('alt'),
+                'Unexpected badge alt text');
+        }
+    },
+
     test_results_display_escaped: function () {
         this.picker.render();
         this.picker.set('results', [

=== modified file 'src-py/lazr/js/build.py'
--- src-py/lazr/js/build.py	2011-05-20 14:54:05 +0000
+++ src-py/lazr/js/build.py	2011-06-03 06:39:38 +0000
@@ -13,10 +13,13 @@
 from glob import glob
 
 import cssutils
+import pkg_resources
 
 HERE = os.path.dirname(__file__)
 BUILD_DIR = os.path.normpath(os.path.join(HERE, '..', '..', '..', 'build'))
-SRC_DIR = os.path.normpath(os.path.join(HERE, '..', '..', '..'))
+DEFAULT_SRC_DIR = os.path.normpath(os.path.join(HERE, '..', '..', '..'))
+PKG_SRC_DIR = pkg_resources.resource_filename(
+    pkg_resources.Requirement.parse("lazr-js"), "lazrjs")
 
 ESCAPE_STAR_PROPERTY_RE = re.compile(r'\*([a-zA-Z0-9_-]+):')
 UNESCAPE_STAR_PROPERTY_RE = re.compile(r'([a-zA-Z0-9_-]+)_ie_star_hack:')
@@ -186,7 +189,7 @@
 
 class Builder:
 
-    def __init__(self, name='lazr', build_dir=BUILD_DIR, src_dir=SRC_DIR,
+    def __init__(self, name='lazr', build_dir=BUILD_DIR, src_dir=PKG_SRC_DIR,
                  extra_files=None, exclude_regex='', file_type='raw',
                  copy_yui_to=BUILD_DIR):
         """Create a new Builder.
@@ -210,6 +213,11 @@
         self.name = name
         self.build_dir = build_dir
         self.src_dir = src_dir
+        # We need to support the case where this is being invoked directly
+        # from source rather than a package. If this is the case, the package
+        # src directory won't exist.
+        if not os.path.exists(src_dir):
+            self.src_dir = DEFAULT_SRC_DIR
         self.built_files = []
         self.skins = {}
         if extra_files is None:
@@ -411,7 +419,7 @@
 
     def copy_yui_package(self):
         """Copy the contents of our 'yui' dir to self.copy_yui_to."""
-        yui_dirs = glob(os.path.join(SRC_DIR, 'yui') + '/*')
+        yui_dirs = glob(os.path.join(self.src_dir, 'yui') + '/*')
         for dir in yui_dirs:
             dir_name = os.path.split(dir)[-1]
             dst_dir = os.path.join(self.copy_yui_to, dir_name)
@@ -450,7 +458,7 @@
         '-b', '--builddir', dest='build_dir', default=BUILD_DIR,
         help=('The directory that should contain built files.'))
     parser.add_option(
-        '-s', '--srcdir', dest='src_dir', default=SRC_DIR,
+        '-s', '--srcdir', dest='src_dir', default=PKG_SRC_DIR,
         help=('The directory containing the src files.'))
     parser.add_option(
         '-x', '--exclude', dest='exclude', default='',