launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03875
[Merge] lp:~jcsackett/launchpad/oh-oh-pick-me-pick-me-3 into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/oh-oh-pick-me-pick-me-3 into lp:launchpad with lp:~jcsackett/launchpad/oh-oh-pick-me-pick-me-2 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/oh-oh-pick-me-pick-me-3/+merge/63885
Summary
=======
Shortly after the person picker widget was created and added to the web ui in oh-oh-pick-me-pick-me-(1,2), a new picker widget was created for launchpad extending the lazr picker to add launchpad specific data.
This branch retargets personpicker to extend that new picker. As a byproduct, both pickers were moved into a new javascript file, lib/lp/app/javascript/widgets.js in order to deal with circular dependencies.
Some drive by fixes were made to the personpicker as well.
Preimplementation
=================
Spoke with Curtis Hovey and Ian Booth.
Implementation
==============
lib/canonical/launchpad/webapp/configure.zcml
---------------------------------------------
Drive by fix; changed the field for the personpicker popup.
lib/lp/app/javascript/picker.js
lib/lp/app/javascript/widgets.js
lib/lp/app/javascript/tests/test_personpicker.js
lib/lp/app/javascript/tests/test_personpicker.html
------------------------------------------------
Migrated the new lp picker and personpicker to lib/lp/app/javascript/widgets.js. Moved the personpicker yui tests to the app javascript module along with personpicker. Updated the personpicker js to extend the lp picker js. Lastly, did a quick fix to change the placement of a paren in the lp picker.
Tests
=====
firefox lib/lp/app/javascript/tests/test_personpicker.html
QA
==
qa-untestable; this will be landed as part of oh-oh-pick-me-pick-me-2, so that lands completely useable. Since that branch was already reviewed and this is dealing with separate issues, it seemed best to submit as a different branch.
Lint
====
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical/launchpad/webapp/configure.zcml
lib/lp/app/javascript/picker.js
lib/lp/app/javascript/widgets.js
lib/lp/app/javascript/tests/test_personpicker.html
lib/lp/app/javascript/tests/test_personpicker.js
--
https://code.launchpad.net/~jcsackett/launchpad/oh-oh-pick-me-pick-me-3/+merge/63885
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/oh-oh-pick-me-pick-me-3 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml 2011-05-27 21:03:22 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml 2011-06-08 15:31:34 +0000
@@ -399,7 +399,7 @@
<!-- Define the widget used by PublicPersonChoice fields. -->
<view
type="zope.publisher.interfaces.browser.IBrowserRequest"
- for="lp.services.fields.PublicPersonChoice
+ for="lp.services.fields.PersonChoice
canonical.launchpad.webapp.vocabulary.IHugeVocabulary"
provides="zope.app.form.interfaces.IInputWidget"
factory="lp.app.widgets.popup.PersonPickerWidget"
=== modified file 'lib/lp/app/javascript/picker.js'
--- lib/lp/app/javascript/picker.js 2011-06-06 03:24:06 +0000
+++ lib/lp/app/javascript/picker.js 2011-06-08 15:31:34 +0000
@@ -328,47 +328,6 @@
Y.on('change', copy_selected_value, select_menu);
};
-
-/**
- * Extend the lazr-js Picker.
- */
-var Picker = function() {
- Picker.superclass.constructor.apply(this, arguments);
-};
-
-Y.extend(Picker, Y.lazr.Picker, {
- // We want to render alt title slightly differently.
- _renderTitleUI: function(data) {
- var li_title = Y.Node.create(
- '<span></span>').addClass(Y.lazr.Picker.C_RESULT_TITLE);
- var title = this._text_or_link(
- data.title, data.title_link, data.link_css);
- li_title.appendChild(title);
- if (data.alt_title) {
- var alt_link = null;
- if (data.alt_title_link) {
- alt_link =Y.Node.create('<a></a>')
- .addClass(data.link_css)
- .addClass('discreet');
- alt_link.set('text', " Details...")
- .set('href', data.alt_title_link);
- Y.on('click', function(e) {
- e.halt();
- window.open(data.alt_title_link);
- }, alt_link);
- }
- li_title.appendChild(' (');
- li_title.appendChild(document.createTextNode(data.alt_title));
- if (alt_link !== null)
- li_title.appendChild(alt_link);
- li_title.appendChild(')');
- }
- return li_title;
- }
-});
-Picker.NAME = 'picker';
-namespace.Picker = Picker;
-
/**
* Creates a picker widget that has already been rendered and hidden.
*
@@ -397,6 +356,9 @@
if (config.step_title !== undefined) {
step_title = config.step_title;
}
+ if (config.picker_type !== undefined) {
+ picker_type = config.picker_type;
+ }
}
if (typeof vocabulary !== 'string' && typeof vocabulary !== 'object') {
@@ -417,7 +379,13 @@
zIndex: 1000,
visible: false
});
- var picker = new Picker(new_config);
+
+ var picker = null;
+ if (picker_type === 'person') {
+ picker = new Y.lp.app.widgets.PersonPicker(new_config);
+ } else {
+ picker = new Y.lp.app.widgets.Picker(new_config);
+ }
// We don't want the Y.lazr.Picker default save to fire since this hides
// the form. We want to do this ourselves after any validation has had a
@@ -540,5 +508,5 @@
}, "0.1", {"requires": [
"io", "dom", "dump", "event", "lazr.picker", "lazr.activator",
- "json-parse", "lp.client"
+ "json-parse", "lp.client", "lp.app.widgets"
]});
=== renamed file 'lib/lp/registry/javascript/tests/test_personpicker.html' => 'lib/lp/app/javascript/tests/test_personpicker.html'
--- lib/lp/registry/javascript/tests/test_personpicker.html 2011-06-01 13:59:43 +0000
+++ lib/lp/app/javascript/tests/test_personpicker.html 2011-06-08 15:31:34 +0000
@@ -17,7 +17,7 @@
<script type="text/javascript" src="../../../app/javascript/lp.js"></script>
<!-- The module under test -->
- <script type="text/javascript" src="../personpicker.js"></script>
+ <script type="text/javascript" src="../widgets.js"></script>
<!-- The test suite -->
<script type="text/javascript" src="test_personpicker.js"></script>
=== renamed file 'lib/lp/registry/javascript/tests/test_personpicker.js' => 'lib/lp/app/javascript/tests/test_personpicker.js'
--- lib/lp/registry/javascript/tests/test_personpicker.js 2011-05-31 21:18:08 +0000
+++ lib/lp/app/javascript/tests/test_personpicker.js 2011-06-08 15:31:34 +0000
@@ -6,10 +6,10 @@
base: '../../../../canonical/launchpad/icing/yui/',
filter: 'raw', combine: false,
fetchCSS: false
- }).use('test', 'console', 'plugin', 'lp.registry.personpicker',
+ }).use('test', 'console', 'plugin', 'lp.app.widgets',
'node-event-simulate', function(Y) {
- var suite = new Y.Test.Suite("lp.registry.personpicker Tests");
+ var suite = new Y.Test.Suite("lp.app.widgets.PersonPicker Tests");
suite.add(new Y.Test.Case({
name: 'personpicker',
@@ -18,26 +18,26 @@
window.LP = {
links: {me: '/~no-one'},
cache: {}
- }
+ };
},
test_render: function () {
- var personpicker = new Y.lp.registry.personpicker.PersonPicker();
+ var personpicker = new Y.lp.app.widgets.PersonPicker();
personpicker.render();
personpicker.show();
-
+
// The extra buttons section exists
Y.Assert.isNotNull(Y.one('.extra-form-buttons'));
Y.Assert.isNotNull(Y.one('.yui-picker-remove-button'));
Y.Assert.isNotNull(Y.one('.yui-picker-assign-me-button'));
},
-
+
test_buttons: function () {
- var personpicker = new Y.lp.registry.personpicker.PersonPicker();
+ var personpicker = new Y.lp.app.widgets.PersonPicker();
personpicker.render();
personpicker.show();
-
- // Patch the picker so the assign_me and remove methods can be
+
+ // Patch the picker so the assign_me and remove methods can be
// tested.
var data = null;
personpicker.on('save', function (result) {
=== renamed file 'lib/lp/registry/javascript/personpicker.js' => 'lib/lp/app/javascript/widgets.js'
--- lib/lp/registry/javascript/personpicker.js 2011-05-31 18:27:50 +0000
+++ lib/lp/app/javascript/widgets.js 2011-06-08 15:31:34 +0000
@@ -1,21 +1,62 @@
/* Copyright 2011 Canonical Ltd. This software is licensed under the
* GNU Affero General Public License version 3 (see the file LICENSE).
*
- * @namespace Y.lp.registry.personpicker
- * @requires lazr.picker
+ * @namespace Y.lp.app.widgets
+ * @requires Y.lazr.picker
*/
-YUI.add('lp.registry.personpicker', function(Y) {
-var namespace = Y.namespace('lp.registry.personpicker');
+YUI.add('lp.app.widgets', function(Y) {
+var namespace = Y.namespace('lp.app.widgets');
-var footer_label = ".yui3-picker-footer-slot"
+var footer_label = ".yui3-picker-footer-slot";
var PersonPicker = function() {
- PersonPicker.superclass.constructor.apply(this, arguments);
-
+ this.superclass.constructor.apply(this, arguments);
this._extra_buttons = Y.Node.create('<div class="extra-form-buttons"/>');
};
-Y.extend(PersonPicker, Y.lazr.Picker, {
+/**
+ * Extend the lazr-js Picker.
+ */
+var Picker = function() {
+ this.superclass.constructor.apply(this, arguments);
+};
+
+Y.extend(Picker, Y.lazr.Picker, {
+ // We want to render alt title slightly differently.
+ _renderTitleUI: function(data) {
+ var li_title = Y.Node.create(
+ '<span></span>').addClass(Y.lazr.Picker.C_RESULT_TITLE);
+ var title = this._text_or_link(
+ data.title, data.title_link, data.link_css);
+ li_title.appendChild(title);
+ if (data.alt_title) {
+ var alt_link = null;
+ if (data.alt_title_link) {
+ alt_link =Y.Node.create('<a></a>')
+ .addClass(data.link_css)
+ .addClass('discreet');
+ alt_link.set('text', " Details...")
+ .set('href', data.alt_title_link);
+ Y.on('click', function(e) {
+ e.halt();
+ window.open(data.alt_title_link);
+ }, alt_link);
+ }
+ li_title.appendChild(' (');
+ li_title.appendChild(document.createTextNode(data.alt_title));
+ li_title.appendChild(')');
+ if (alt_link !== null) {
+ li_title.appendChild(alt_link);
+ }
+ }
+ return li_title;
+ }
+});
+
+Picker.NAME = 'picker';
+namespace.Picker = Picker;
+
+Y.extend(PersonPicker, namespace.Picker, {
hide: function() {
this.get('boundingBox').setStyle('display', 'none');
},
@@ -36,29 +77,24 @@
renderUI: function() {
this.constructor.superclass.renderUI.call(this);
var remove_button, assign_me_button;
- //# TODO config should set extrabuttons
- var show_remove_button = true;
- var show_assign_me_button = true;
var remove_button_text = "Remove assignee";
var assign_me_button_text = "Assign me";
- if (show_remove_button) {
- remove_button = Y.Node.create(
- '<a class="yui-picker-remove-button bg-image" ' +
- 'href="javascript:void(0)" ' +
- 'style="background-image: url(/@@/remove); padding-right: 1em">' +
- remove_button_text + '</a>');
- remove_button.on('click', this.remove, this);
- this._extra_buttons.appendChild(remove_button);
- }
- if (show_assign_me_button) {
- assign_me_button = Y.Node.create(
- '<a class="yui-picker-assign-me-button bg-image" ' +
- 'href="javascript:void(0)" ' +
- 'style="background-image: url(/@@/person)">' +
- assign_me_button_text + '</a>');
- assign_me_button.on('click', this.assign_me, this);
- this._extra_buttons.appendChild(assign_me_button);
- }
+
+ remove_button = Y.Node.create(
+ '<a class="yui-picker-remove-button bg-image" ' +
+ 'href="javascript:void(0)" ' +
+ 'style="background-image: url(/@@/remove); padding-right: 1em">' +
+ remove_button_text + '</a>');
+ remove_button.on('click', this.remove, this);
+ this._extra_buttons.appendChild(remove_button);
+
+ assign_me_button = Y.Node.create(
+ '<a class="yui-picker-assign-me-button bg-image" ' +
+ 'href="javascript:void(0)" ' +
+ 'style="background-image: url(/@@/person)">' +
+ assign_me_button_text + '</a>');
+ assign_me_button.on('click', this.assign_me, this);
+ this._extra_buttons.appendChild(assign_me_button);
},
syncUI: function() {
@@ -69,6 +105,6 @@
}
});
PersonPicker.NAME = 'person-picker';
-namespace.PersonPicker = PersonPicker
+namespace.PersonPicker = PersonPicker;
-}, "0.1", {"requires": ['lazr.picker']});
+}, "0.1", {"requires": ["lazr.picker"]});
=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py 2011-06-01 13:59:43 +0000
+++ lib/lp/app/widgets/popup.py 2011-06-08 15:31:34 +0000
@@ -18,6 +18,7 @@
from canonical.launchpad.webapp import canonical_url
from lp.app.browser.stringformatter import FormattersAPI
+from lp.services.features import getFeatureFlag
from lp.services.propertycache import cachedproperty
@@ -26,6 +27,8 @@
__call__ = ViewPageTemplateFile('templates/form-picker.pt')
+ picker_type = 'default'
+
popup_name = 'popup-vocabulary-picker'
# Override inherited attributes for the form field.
@@ -157,6 +160,16 @@
include_create_team_link = False
+ @property
+ def picker_type(self):
+ # This is a method for now so we can block the use of the new
+ # person picker js behind our picker_enhancments feature flag.
+ if bool(getFeatureFlag('disclosure.picker_enhancements.enabled')):
+ picker_type = 'person'
+ else:
+ picker_type = 'default'
+ return picker_type
+
def chooseLink(self):
link = super(PersonPickerWidget, self).chooseLink()
if self.include_create_team_link:
@@ -172,7 +185,6 @@
class BugTrackerPickerWidget(VocabularyPickerWidget):
__call__ = ViewPageTemplateFile('templates/bugtracker-picker.pt')
-
link_template = """
or (<a id="create-bugtracker-link"
href="/bugs/bugtrackers/+newbugtracker"
=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt 2011-06-01 20:55:24 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt 2011-06-08 15:31:34 +0000
@@ -40,7 +40,8 @@
var config = {
header: ${view/header_text},
step_title: ${view/step_title_text},
- extra_no_results_message: ${view/extra_no_results_message}
+ extra_no_results_message: ${view/extra_no_results_message},
+ picker_type: '${view/picker_type}'
};
var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
if (config.extra_no_results_message !== null) {
