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

Proposed by Guilherme Salgado
Status: Merged
Merged at revision: not available
Proposed branch: lp:~salgado/lazr-js/bug-482235
Merge into: lp:lazr-js
Diff against target: 248 lines (+107/-18)
3 files modified
examples/picker/index.html (+30/-12)
src-js/lazrjs/picker/picker.js (+41/-2)
src-js/lazrjs/picker/tests/picker.js (+36/-4)
To merge this branch: bzr merge lp:~salgado/lazr-js/bug-482235
Reviewer Review Type Date Requested Status
Māris Fogels (community) Approve
Björn Tillenius Pending
Review via email: mp+16349@code.launchpad.net

This proposal supersedes a proposal from 2009-12-10.

To post a comment you must log in.
Revision history for this message
Guilherme Salgado (salgado) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Björn Tillenius (bjornt) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Māris Fogels (mars) wrote : Posted in a previous version of this proposal

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.

review: Abstain
Revision history for this message
Guilherme Salgado (salgado) wrote : Posted in a previous version of this proposal
Download full text (3.8 KiB)

> 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...

Read more...

Revision history for this message
Māris Fogels (mars) wrote : Posted in a previous version of this proposal

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.

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

On Fri, Dec 18, 2009 at 06:30:31PM -0000, Guilherme Salgado wrote:
> === modified file 'src-js/lazrjs/picker/picker.js'
> --- src-js/lazrjs/picker/picker.js 2009-11-24 00:09:41 +0000
> +++ src-js/lazrjs/picker/picker.js 2009-12-18 18:30:30 +0000
> @@ -863,10 +863,39 @@
> }
> };
>
> +
> +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().
> + // XXX: For that reason we need callsites to provide an element ID
> + // rather than a selector here.
> + var input = Y.DOM.byId(config.input_id);

This is not quite true. You can use CSS3 selectors if you use attribute
selectors. For example:

    var input = Y.one('[id="some.id"]');

The rest looks good in general, it looks nice. Although I'm missing some
documenation. Do you plan to add some?

--
Björn Tillenius | https://launchpad.net/~bjornt

lp:~salgado/lazr-js/bug-482235 updated
167. By Guilherme Salgado

Move Picker and TextFieldPickerPlugin under tha lazr (Y.lazr) namespace

168. By Guilherme Salgado

Docs and minor changes suggested by Bjorn

169. By Guilherme Salgado

Add an example showing how to use Y.lazr.TextFieldPickerPlugin

Revision history for this message
Guilherme Salgado (salgado) wrote :

Hi Bjorn,

On Mon, 2009-12-21 at 12:12 +0000, Björn Tillenius wrote:
> On Fri, Dec 18, 2009 at 06:30:31PM -0000, Guilherme Salgado wrote:
[...]
> > +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().
> > + // XXX: For that reason we need callsites to provide an element ID
> > + // rather than a selector here.
> > + var input = Y.DOM.byId(config.input_id);
>
> This is not quite true. You can use CSS3 selectors if you use attribute
> selectors. For example:
>
> var input = Y.one('[id="some.id"]');

I didn't know that I could use attribute selectors to workaround that,
and the comment was just copied from the LP code that does what is now
done by the plugin.

I've changed it the plugin to expect any CSS selector, instead of an ID,
added some docs and an example.

        http://paste.ubuntu.com/344218/
        Example: http://paste.ubuntu.com/344220/

And I've also moved the Picker from the Y namespace into the Y.lazr one,
after checking with Maris that that's where it belongs.

        http://paste.ubuntu.com/344216/

How does it look now?

--
Guilherme Salgado <email address hidden>

Revision history for this message
Māris Fogels (mars) wrote :

Hi Salgado,

Your changes look good. I have some comments about the implementation.

- You should use Y.one() instead of Y.get()
- Are you comfortable using a private member reference to get the raw DOM node (node._node)? There is a proper API function for that: Y.Node.getDOMNode().

- Just a comment: not in your code, but in the picker example page, I do not like the way the save event callback functions use array references and class-static variables to access the event details. They should be using objects. Do not worry about fixing it. The picker will have to be cleaned up and refactored at some point in the future.

Looks good. With changing Y.get() to Y.one(), r=mars.

Maris

review: Approve
Revision history for this message
Guilherme Salgado (salgado) wrote :

Thanks Maris!

I changed my code (and test) to use Y.Node.getDOMNode() instead of
accessing _node directly. And replaced Y.get with Y.one -- for some
reason I always forget which one is deprecated.

lp:~salgado/lazr-js/bug-482235 updated
170. By Guilherme Salgado

A couple minor tweaks suggested by Maris

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'examples/picker/index.html'
--- examples/picker/index.html 2009-11-30 19:07:28 +0000
+++ examples/picker/index.html 2009-12-22 16:14:11 +0000
@@ -21,9 +21,11 @@
2121
22 <script type="text/javascript">22 <script type="text/javascript">
2323
24YUI(LAZR_YUI_CONFIG).use('lazr.picker', 'dump', function(Y) {24 YUI(LAZR_YUI_CONFIG).use('lazr.picker', 'dump', 'plugin', function(Y) {
25 // Parse the content of the textarea in the data structure25
26 // expected by the widget.26 var Picker = Y.lazr.Picker;
27 // Parse the content of the textarea in the data structure
28 // expected by the widget.
27 function parse_items () {29 function parse_items () {
28 var items_text = Y.Lang.trim(Y.get('#items').get('value'));30 var items_text = Y.Lang.trim(Y.get('#items').get('value'));
29 var lines = items_text.split(/\r\n|\r|\n/);31 var lines = items_text.split(/\r\n|\r|\n/);
@@ -71,7 +73,7 @@
71 }73 }
7274
7375
74 var picker = new Y.Picker({76 var picker = new Picker({
75 picker_activator: '#show-widget',77 picker_activator: '#show-widget',
76 clear_on_cancel: true,78 clear_on_cancel: true,
77 align: {79 align: {
@@ -84,9 +86,11 @@
84 zIndex: 100086 zIndex: 1000
85 });87 });
8688
89 picker.plug(
90 Y.lazr.TextFieldPickerPlugin, {input_element: '#initval'});
87 picker.subscribe('save', function (e) {91 picker.subscribe('save', function (e) {
88 Y.log('Got save event.');92 Y.log('Got save event.');
89 var result = e.details[Y.Picker.SAVE_RESULT];93 var result = e.details[Picker.SAVE_RESULT];
90 Y.get('#selected-item').set(94 Y.get('#selected-item').set(
91 'innerHTML', result['title'] + " (" + result['value'] + ')');95 'innerHTML', result['title'] + " (" + result['value'] + ')');
92 });96 });
@@ -106,14 +110,14 @@
106 // the demo look better. A real application wouldn't delay the110 // the demo look better. A real application wouldn't delay the
107 // results just so you can the spinner.111 // results just so you can the spinner.
108 var spin_interval;112 var spin_interval;
109 if (e.details[Y.Picker.SELECTED_BATCH_VALUE] !== undefined) {113 if (e.details[Picker.SELECTED_BATCH_VALUE] !== undefined) {
110 spin_interval = 500;114 spin_interval = 500;
111 } else {115 } else {
112 spin_interval = 2000;116 spin_interval = 2000;
113 }117 }
114 Y.later(spin_interval, null, function () {118 Y.later(spin_interval, null, function () {
115 var results = search_items(e.details[Y.Picker.SEARCH_STRING]);119 var results = search_items(e.details[Picker.SEARCH_STRING]);
116 var selected_batch = e.details[Y.Picker.SELECTED_BATCH_VALUE];120 var selected_batch = e.details[Picker.SELECTED_BATCH_VALUE];
117 var simple_batching = Y.get('#page-label-1').get('checked');121 var simple_batching = Y.get('#page-label-1').get('checked');
118122
119 if (simple_batching) {123 if (simple_batching) {
@@ -228,6 +232,9 @@
228 <b>Last item selected:</b> <span id="selected-item">None</span>232 <b>Last item selected:</b> <span id="selected-item">None</span>
229 </div>233 </div>
230 <div>234 <div>
235 <b>Initial value:</b> <input type="text" id="initval" value="" />
236 </div>
237 <div>
231 <b>Minimum search characters:</b>238 <b>Minimum search characters:</b>
232 <input type="text" id="min-search-chars" value="3" />239 <input type="text" id="min-search-chars" value="3" />
233 </div>240 </div>
@@ -301,7 +308,7 @@
301 rather than creating a new Picker every time you want to display it.308 rather than creating a new Picker every time you want to display it.
302 </p>309 </p>
303 <pre>310 <pre>
304 var picker = new Y.Picker({311 var picker = new Picker({
305 progressbar: true,312 progressbar: true,
306 progress: 100,313 progress: 100,
307 headerContent: '&lt;h2&gt;Picker example&lt;/h2&gt;',314 headerContent: '&lt;h2&gt;Picker example&lt;/h2&gt;',
@@ -370,8 +377,8 @@
370 </p>377 </p>
371 <pre>378 <pre>
372 picker.after('search', function(e) {379 picker.after('search', function(e) {
373 var search_text = e.details[Y.Picker.SEARCH_STRING];380 var search_text = e.details[Picker.SEARCH_STRING];
374 var selected_batch = e.details[Y.Picker.SELECTED_BATCH_VALUE];381 var selected_batch = e.details[Picker.SELECTED_BATCH_VALUE];
375 var result_batches = my_perform_search(search_text);382 var result_batches = my_perform_search(search_text);
376383
377 if (selected_batch === undefined) {384 if (selected_batch === undefined) {
@@ -406,7 +413,7 @@
406 </p>413 </p>
407 <pre>414 <pre>
408 picker.subscribe('save', function(e) {415 picker.subscribe('save', function(e) {
409 var result = e.details[Y.Picker.SAVE_RESULT];416 var result = e.details[Picker.SAVE_RESULT];
410 Y.log('Got save event with result: ' + result);417 Y.log('Got save event with result: ' + result);
411 });418 });
412 </pre>419 </pre>
@@ -420,5 +427,16 @@
420 Y.log('Got cancel event');427 Y.log('Got cancel event');
421 });428 });
422 </pre>429 </pre>
430 <h3>Associating the picker to a text input</h3>
431 <p>Sometimes it's desirable to have a picker associated to a text input so
432 that the value selected from the picker's search results is copied back
433 to that text input when the save event is fired. The
434 TextFieldPickerPlugin does that and also extends the picker to take the
435 initial value of its search input from the associated text input.
436 </p>
437 <pre>
438 picker.plug(
439 Y.lazr.TextFieldPickerPlugin, {input_element: '#initval'});
440 </pre>
423</body>441</body>
424</html>442</html>
425443
=== modified file 'src-js/lazrjs/picker/picker.js'
--- src-js/lazrjs/picker/picker.js 2009-11-24 00:09:41 +0000
+++ src-js/lazrjs/picker/picker.js 2009-12-22 16:14:11 +0000
@@ -863,10 +863,49 @@
863 }863 }
864};864};
865865
866Y.Picker = Picker;866
867/**
868 * This plugin is used to associate a picker instance to an input element of
869 * the DOM. When the picker is shown, it takes its initial value from that
870 * element and when the save event is fired, the value of the chosen item
871 * (from the picker's list of results) is copied to that element.
872 *
873 * Also, this plugin expects a single attribute (input_element) in the
874 * config passed to its constructor, which defines the element that will be
875 * associated with the picker.
876 *
877 * @class TextFieldPickerPlugin
878 * @extends Y.Plugin.Base
879 * @constructor
880 */
881
882function TextFieldPickerPlugin(config) {
883 TextFieldPickerPlugin.superclass.constructor.apply(this, arguments);
884}
885
886TextFieldPickerPlugin.NAME = 'TextFieldPickerPlugin';
887TextFieldPickerPlugin.NS = 'txtpicker';
888
889Y.extend(TextFieldPickerPlugin, Y.Plugin.Base, {
890 initializer: function(config) {
891 var input = Y.Node.getDOMNode(Y.one(config.input_element));
892 this.doAfter('save', function (e) {
893 result = e.details[Y.lazr.Picker.SAVE_RESULT];
894 input.value = result.value;
895 });
896 this.doAfter('show', function() {
897 if ( input.value ) {
898 this.get('host')._search_input.set('value', input.value);
899 }
900 });
901 }
902});
903
904Y.lazr.Picker = Picker;
905Y.lazr.TextFieldPickerPlugin = TextFieldPickerPlugin;
867906
868}, "0.1", {"skinnable": true,907}, "0.1", {"skinnable": true,
869 "requires": ["oop", "event", "event-focus", "node",908 "requires": ["oop", "event", "event-focus", "node", "plugin",
870 "substitute", "widget", "widget-stdmod",909 "substitute", "widget", "widget-stdmod",
871 "lazr.overlay", "lazr.anim", "lazr.base"]910 "lazr.overlay", "lazr.anim", "lazr.base"]
872});911});
873912
=== modified file 'src-js/lazrjs/picker/tests/picker.js'
--- src-js/lazrjs/picker/tests/picker.js 2009-11-30 19:07:28 +0000
+++ src-js/lazrjs/picker/tests/picker.js 2009-12-22 16:14:11 +0000
@@ -39,7 +39,7 @@
39 name: 'picker_basics',39 name: 'picker_basics',
4040
41 setUp: function() {41 setUp: function() {
42 this.picker = new Y.Picker();42 this.picker = new Y.lazr.Picker();
43 },43 },
4444
45 tearDown: function() {45 tearDown: function() {
@@ -48,7 +48,7 @@
4848
49 test_picker_can_be_instantiated: function() {49 test_picker_can_be_instantiated: function() {
50 Assert.isInstanceOf(50 Assert.isInstanceOf(
51 Y.Picker, this.picker, "Picker failed to be instantiated");51 Y.lazr.Picker, this.picker, "Picker failed to be instantiated");
52 },52 },
5353
54 test_picker_is_stackable: function() {54 test_picker_is_stackable: function() {
@@ -406,7 +406,7 @@
406 },406 },
407407
408 test_save_does_not_clear_widget_when_clear_on_save_is_false: function () {408 test_save_does_not_clear_widget_when_clear_on_save_is_false: function () {
409 picker = new Y.Picker({clear_on_save: false});409 picker = new Y.lazr.Picker({clear_on_save: false});
410 picker.render();410 picker.render();
411411
412 picker._search_input.set('value', 'foo');412 picker._search_input.set('value', 'foo');
@@ -427,7 +427,7 @@
427 },427 },
428428
429 test_cancel_event_clears_widget_when_clear_on_cancel_is_true: function () {429 test_cancel_event_clears_widget_when_clear_on_cancel_is_true: function () {
430 picker = new Y.Picker({clear_on_cancel: true});430 picker = new Y.lazr.Picker({clear_on_cancel: true});
431 picker.render();431 picker.render();
432432
433 picker._search_input.set('value', 'foo');433 picker._search_input.set('value', 'foo');
@@ -841,6 +841,38 @@
841 this.picker.set('search_mode', false);841 this.picker.set('search_mode', false);
842 Assert.isTrue(got_focus, "focus didn't go to the search input.");842 Assert.isTrue(got_focus, "focus didn't go to the search input.");
843 }843 }
844}));
845
846suite.add(new Y.Test.Case({
847
848 name: 'picker_text_field_plugin',
849
850 setUp: function() {
851 var node = Y.one(document.body).appendChild(
852 Y.Node.create('<input id="field.initval" value="foo" />'));
853 this.picker = new Y.lazr.Picker();
854 this.picker.plug(Y.lazr.TextFieldPickerPlugin,
855 {input_element: '[id="field.initval"]'});
856 },
857
858 tearDown: function() {
859 cleanup_widget(this.picker);
860 },
861
862 test_TextFieldPickerPlugin_initial_value: function () {
863 this.picker.render();
864 this.picker.show();
865 Assert.areEqual('foo', this.picker._search_input.get('value'));
866 },
867
868 test_TextFieldPickerPlugin_selected_item_is_saved: function () {
869 this.picker.set('results', [{'title': 'Object 1', value: 'first'}]);
870 this.picker.render();
871 simulate(
872 this.picker.get('boundingBox'), '.yui-picker-results li', 'click');
873 Assert.areEqual(
874 'first', Y.Node.getDOMNode(Y.one('[id="field.initval"]')).value);
875 },
844876
845}));877}));
846878

Subscribers

People subscribed via source and target branches