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

Revision history for this message
Björn Tillenius (bjornt) wrote :

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;
    });

review: Abstain

« Back to merge proposal