Merge lp:~salgado/lazr-js/bug-482235 into lp:lazr-js

Add a new attribute (get_initial_value_from) to the picker. When that attribute is set, the initial value for the picker's search input will come from the page element identified by it

The feature itself looks very useful. I have some comments and questions, though. First of all, reading the diff, it looks like it only sets the search text, but doesn't actually search. But I assume that it does perform an actual search, no?

I'm also not so sure about the API. I can see how you chose to add a parameter, since that fits into the current API. However, I think it's worth thinking about, whether we can do something better. The reason I'm a bit skeptic to having yet another parameter, is that you mostly want to use this when you have a text input field, right? And when you have a text input field, you mostly always want this functionality, and you mostly always want to put pack the search result from the picker into the text field, right? With the current API, that means that every time you connect the picker to a text field, you have to do two things, specifying the same text field twice.

I want us to specify the text field only once. A possible solution would be to use the plugin API to achieve this. So that you have something like:

    var picker = new Y.Picker();
    picker.plug(TextFieldPickerPlugin('#text-field'));

Compare that to the existing API which looks something like this:

    var picker = new Y.Picker({get_initial_value_from: '#text-field'});
    picker.subscribe('save', function (e) {
        Y.DOM.byId('#text-field').value = e.details[Y.Picker.SAVE_RESULT].value;
    });

I agree, and as I stated in the original bug report, the API could be improved.

I would not mind building the initial_value attribute as described in the bug report into the lazr-js class, but I think Björn's suggestion works too.

Björn's idea can be simplified a bit. Assuming we want the functionality across all of Launchpad, you can plug into the Picker class itself using the class-level static .plug() method.

Instead of this:

>
> var picker = new Y.Picker();
> picker.plug(TextFieldPickerPlugin('#text-field'));
>

You do this:

Y.Picker.plug(TextFieldPickerPlugin);
var picker = Y.Picker({
  ...
  use_value_from: '#foobar' // Our new attribute
});

The class-level plugin could then add the attributes it needs dynamically.

> The feature itself looks very useful. I have some comments and questions,
> though. First of all, reading the diff, it looks like it only sets the search
> text, but doesn't actually search. But I assume that it does perform an actual
> search, no?

Currently, it doesn't but that's trivial to add.

>
> I'm also not so sure about the API. I can see how you chose to add a
> parameter, since that fits into the current API. However, I think it's worth
> thinking about, whether we can do something better. The reason I'm a bit
> skeptic to having yet another parameter, is that you mostly want to use this
> when you have a text input field, right? And when you have a text input field,
> you mostly always want this functionality, and you mostly always want to put
> pack the search result from the picker into the text field, right? With the
> current API, that means that every time you connect the picker to a text
> field, you have to do two things, specifying the same text field twice.
>
> I want us to specify the text field only once. A possible solution would be to
> use the plugin API to achieve this. So that you have something like:
>
> var picker = new Y.Picker();
> picker.plug(TextFieldPickerPlugin('#text-field'));

Below is what I came up with, but notice the ID of the text field is not passed to TexFieldPickerPlugin (I didn't find out how to do that).

One thing that's not clear to me is the advantage of using the plugin API instead of just subclassing the picker and adding the functionality to the subclass? AFAICT, subclassing would work just fine and it would be certainly simpler.

=== modified file 'lib/canonical/widgets/templates/vocabulary-picker.js'
--- lib/canonical/widgets/templates/vocabulary-picker.js 2009-09-21 12:27:14 +0000
+++ lib/canonical/widgets/templates/vocabulary-picker.js 2009-12-11 19:54:11 +0000
@@ -1,4 +1,4 @@
-YUI().use('node', 'lp.picker', function(Y) {
+YUI().use('node', 'lp.picker', 'plugin', function(Y) {
     if (Y.UA.ie) {
         return;
     }
@@ -6,18 +6,39 @@
     // Args from python.
     var args = %s;

+ function TextFieldPickerPlugin(config) {
+ TextFieldPickerPlugin.superclass.constructor.apply(this, arguments);
+ }
+
+ TextFieldPickerPlugin.NAME = 'TextFieldPickerPlugin';
+ TextFieldPickerPlugin.NS = 'txtpicker';
+
+ Y.extend(TextFieldPickerPlugin, Y.Plugin.Base, {
+ initializer: function(config) {
+ // Y.one() uses CSS3 selectors which don't work with ids
+ // containing a period, so we have to use Y.DOM.byId().
+ var input = Y.DOM.byId(args.input_id);
+ this.doAfter('save', function (e) {
+ result = e.details[Y.Picker.SAVE_RESULT];
+ input.value = result.value;
+ });
+ this.doAfter('show', function() {
+ if ( input.value ) {
+ this.get('host')._search_input.set('value', input.value);
+ }
+ });
+ }
+ });
+
     // The vocabulary picker, created when used for the first time.
     var picker = null;
     function make_picker() {
- var save = function (result) {
- Y.DOM.byId(args...

On Mon, Dec 14, 2009 at 2:32 PM, Guilherme Salgado
<email address hidden> wrote:
>
> One thing that's not clear to me is the advantage of using the plugin API instead of just subclassing the picker and adding the functionality to the subclass?  AFAICT, subclassing would work just fine and it would be certainly simpler.
>

YUI widgets were not designed to be subclassed for the reason are thinking.

Subclassing a YUI widget means literally a new widget built on top of
the old functionality. You must give the widget a new .NAME
attribute, new CSS, a new renderUI, bindUI, and syncUI methods, etc.

YUI leans towards composing widgets to add features, rather than
subclassing to add them. That is what the Plugin API and
Y.Base.build() are for. Thus, that is why I suggested using the
Plugin API or the Extension API. In this case, a Plugin is simplest.