launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03819
[Merge] lp:~sinzui/launchpad/picker-suggestion-menu-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/picker-suggestion-menu-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/picker-suggestion-menu-0/+merge/63260
Use standard events with the picker suggestion widget.
Launchpad bug: https://bugs.launchpad.net/bugs/97266
Pre-implementation: no one
1. Try to retarget an existing bug to the non-existent
"launchpad-infrastructure" project.
2. On the resulting error page, observe that there is a menu of suggestions
with two items: launchpad-development-infrastructure and qprocd.
3. Choose either of the items.
What was expected: The Launchpad ID of the selected item should be copied
into the text field.
What happened: Nothing at all.
This is broken because the implementation assumed that the option element
can have an onclick event, but in standard DOM, it does not because the
select element manages the options. Only gecko-based browsers supported
this wrong behaviour.
--------------------------------------------------------------------
RULES
* Update the script to work with the onchange event of the select
element.
* Add text to explain the appearance of the select widget.
* The option text should use the unique_displayname format of
Display Name (lp-id).
* Use YUI.
* ADDENDUM: Add basic tests to verify the untested suggestion behaviour.
QA
* This is best tested in a webkit or opera based browser.
* https://bugs.qastaging.launchpad.net/launchpad/+bug/97266
* Try to retarget the bug to "launchpad-infrastructure" using
the html form.
* Verify that the suggestion widget shows the matches in this format:
Launchpad Auto Build System (launchpad-buildd)
* Choose an option.
* Verify it appears in the project field.
LINT
lib/lp/app/javascript/picker.js
lib/lp/app/javascript/tests/test_picker.js
lib/lp/app/widgets/templates/form-picker-macros.pt
lib/lp/app/widgets/tests/test_popup.py
TEST
./bin/test -vv -t TestVocabularyPickerWidget
open lib/lp/app/javascript/tests/test_picker.html
^ Windmill does not run with the current version of FF.
IMPLEMENTATION
Lint really hated picker.js. I fixed all the issues. The spinner change
revealed that "!= null" was indeed coercing an undefined value.
Added a new method to copy the select element's value to the text input.
Updated the template to use the new method instead of the untested and
non-standard hack to the option items.
lib/lp/app/javascript/picker.js
lib/lp/app/javascript/tests/test_picker.js
lib/lp/app/widgets/templates/form-picker-macros.pt
Added missing test coverage to verify that the widget renders the javascript
to connection the choose link and suggestion widget. Added a test to verify
that matches are provided by the widget.
lib/lp/app/widgets/tests/test_popup.py
--
https://code.launchpad.net/~sinzui/launchpad/picker-suggestion-menu-0/+merge/63260
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/picker-suggestion-menu-0 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js 2011-05-31 13:01:07 +0000
+++ lib/lp/app/javascript/picker.js 2011-06-02 16:28:26 +0000
@@ -192,8 +192,13 @@
config.temp_spinner.removeClass('unseen');
picker.set('min_search_chars', 0);
picker.fire('search', '');
+<<<<<<< TREE
picker.get('contentBox').one(
'.yui3-picker-search-box').addClass('unseen');
+=======
+ picker.get('contentBox').one(
+ '.yui3-picker-search-box').addClass('unseen');
+>>>>>>> MERGE-SOURCE
}
picker.show();
});
@@ -221,7 +226,11 @@
* Remove the Loading.... spinner (if it exists).
*/
function hide_temporary_spinner(temp_spinner) {
+<<<<<<< TREE
if( temp_spinner !== null ) {
+=======
+ if (temp_spinner !== undefined && temp_spinner !== null) {
+>>>>>>> MERGE-SOURCE
temp_spinner.addClass('unseen');
}
}
@@ -313,6 +322,22 @@
}
}
+
+/*
+ * Connect the onchange event of the select menu to copy the selected value
+ * to the text input.
+ *
+ * @param {Node} select_menu The select menu with suggested matches.
+ * @param {Node} text_input The input field to copy the selected match too.
+ */
+namespace.connect_select_menu = function (select_menu, text_input) {
+ var copy_selected_value = function(e) {
+ text_input.value = select_menu.value;
+ };
+ Y.on('change', copy_selected_value, select_menu);
+ };
+
+
/**
* Creates a picker widget that has already been rendered and hidden.
*
@@ -416,7 +441,12 @@
var batches = [];
var stop = Math.ceil(total_size / BATCH_SIZE);
if (stop > 1) {
+<<<<<<< TREE
for (batch = 0; batch < stop; batch++) {
+=======
+ var i;
+ for (i=0; i<stop; i++) {
+>>>>>>> MERGE-SOURCE
batches.push({
name: batch+1,
value: batch
@@ -483,6 +513,6 @@
};
}, "0.1", {"requires": [
- "io", "dom", "dump", "lazr.picker", "lazr.activator", "json-parse",
- "lp.client"
+ "io", "dom", "dump", "event", "lazr.picker", "lazr.activator",
+ "json-parse", "lp.client"
]});
=== modified file 'lib/lp/app/javascript/tests/test_picker.js'
--- lib/lp/app/javascript/tests/test_picker.js 2011-05-31 13:25:53 +0000
+++ lib/lp/app/javascript/tests/test_picker.js 2011-06-02 16:28:26 +0000
@@ -7,6 +7,7 @@
fetchCSS: false
}).use('test', 'console', 'node', 'lp', 'lp.client', 'escape', 'event',
'event-focus', 'event-simulate', 'lazr.picker','lp.app.picker',
+ 'node-event-simulate',
function(Y) {
var Assert = Y.Assert;
@@ -47,9 +48,13 @@
{"value": "frieda", "title": "Frieda", "css": "sprite-person",
"description": "frieda@xxxxxxxxxxx", "api_uri": "~/frieda"}
];
+ this.select_menu = null;
},
tearDown: function() {
+ if (this.select_menu !== null) {
+ Y.one('body').removeChild(this.select_menu);
+ }
cleanup_widget(this.picker);
},
@@ -181,7 +186,26 @@
Assert.areEqual(
undefined, save_flag.event_has_fired,
"save event wasn't fired.");
- }
+ },
+
+ test_connect_select_menu: function() {
+ // connect_select_menu() connects the select menu's onchange event to
+ // copy the selected value to the text input field.
+ this.select_menu = Y.Node.create(
+ '<select id="field.initval-suggestions"> ' +
+ ' <option value="">Did you mean...</option>' +
+ ' <option value="fnord">Fnord Snarf (fnord)</option>' +
+ '</select>');
+ Y.one('body').appendChild(this.select_menu);
+ var select_menu = Y.DOM.byId('field.initval-suggestions');
+ var text_input = Y.DOM.byId('field.initval');
+ Y.lp.app.picker.connect_select_menu(select_menu, text_input);
+ select_menu.selectedIndex = 1;
+ Y.Event.simulate(select_menu, 'change');
+ Assert.areEqual(
+ 'fnord', text_input.value,
+ "Select menu's onchange handler failed.");
+ }
}));
/*
=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt 2011-04-16 00:52:07 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt 2011-06-02 16:28:26 +0000
@@ -6,16 +6,26 @@
<metal:form-picker define-macro="form-picker">
<tal:input replace="structure view/inputField" />
- <tal:search_results condition="not: view/hasValidInput">
- <select tal:condition="view/matches">
+ <tal:search_results condition="not: view/hasValidInput"
+ define="suggestion_id string:${view/name}-suggestions">
+ <select tal:condition="view/matches"
+ tal:attributes="id string:${suggestion_id}">
+ <option value="">Did you mean...</option>
<option
tal:repeat="match view/matches"
tal:attributes="value match/token;
- selected python:path('match/token') == path('view/formToken');
- onclick string:this.form['${view/name}'].value = this.value"
- tal:content="string:${match/token}: ${match/title}"
+ selected python:path('match/token') == path('view/formToken');"
+ tal:content="string:${match/title} (${match/token})"
/>
</select>
+ <script type="text/javascript" tal:content="string:
+ LPS.use('node', 'lp.app.picker', function(Y) {
+ var text_input = Y.DOM.byId('${view/name}');
+ var select_menu = Y.DOM.byId('${suggestion_id}');
+ Y.lp.app.picker.connect_select_menu(
+ select_menu, text_input);
+ });">
+ </script>
</tal:search_results>
<tal:chooseLink replace="structure view/chooseLink" />
=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py 2011-05-29 23:34:26 +0000
+++ lib/lp/app/widgets/tests/test_popup.py 2011-06-02 16:28:26 +0000
@@ -70,6 +70,9 @@
'field.test_valid.item', picker_widget.input_id)
self.assertEqual(
simplejson.dumps(None), picker_widget.extra_no_results_message)
+ markup = picker_widget()
+ self.assertIn(
+ "Y.lp.app.picker.create('ValidTeamOwner', config);", markup)
def test_widget_fieldname_with_invalid_html_chars(self):
# Check the picker widget is correctly set up for a field which has a
@@ -83,8 +86,29 @@
# The widget name is encoded to get the widget's ID. It must only
# contain valid HTML characters.
self.assertEqual(
- 'show-widget-field-test_invalid_chars-ZmllbGQudGVzdF9pbnZhbGlkX2NoYXJzKw',
+ 'show-widget-field-test_invalid_chars-'
+ 'ZmllbGQudGVzdF9pbnZhbGlkX2NoYXJzKw',
picker_widget.show_widget_id)
self.assertEqual(
'field.test_invalid_chars-ZmllbGQudGVzdF9pbnZhbGlkX2NoYXJzKw',
picker_widget.input_id)
+
+ def test_widget_suggestions(self):
+ # The suggestions menu is shown when the input is invalid and there
+ # are matches to suggest to the user.
+ field = ITest['test_valid.item']
+ bound_field = field.bind(self.context)
+ request = LaunchpadTestRequest(form={'field.test_valid.item': 'foo'})
+ picker_widget = VocabularyPickerWidget(
+ bound_field, self.vocabulary, request)
+ matches = list(picker_widget.matches)
+ self.assertEqual(1, len(matches))
+ self.assertEqual('Foo Bar', matches[0].title)
+ markup = picker_widget()
+ self.assertIn('id="field.test_valid.item-suggestions"', markup)
+ self.assertIn(
+ "Y.DOM.byId('field.test_valid.item-suggestions')", markup)
+ self.assertTextMatchesExpressionIgnoreWhitespace(
+ 'Y.lp.app.picker.connect_select_menu\( '
+ 'select_menu, text_input\);',
+ markup)
