Code review comment for lp:~salgado/lazr-js/bug-482235

> 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.input_id).value = result.value;
- };
         var config = {
             header: args.header,
             step_title: args.step_title,
             extra_no_results_message: args.extra_no_results_message
         };
- var picker = Y.lp.picker.create(args.vocabulary, save, config);
+ var picker = Y.lp.picker.create(args.vocabulary, config);
         if (config.extra_no_results_message !== null) {
             picker.before('resultsChange', function (e) {
                 var new_results = e.details[0].newVal;
@@ -30,6 +51,7 @@
                 }
             });
         }
+ picker.plug(TextFieldPickerPlugin);
         return picker;
     }