Merge lp:~jcsackett/launchpad/picker-patcher-picked-a-patch-of-personpickers into lp:launchpad
Status: | Merged |
---|---|
Approved by: | Curtis Hovey |
Approved revision: | no longer in the source branch. |
Merged at revision: | 13218 |
Proposed branch: | lp:~jcsackett/launchpad/picker-patcher-picked-a-patch-of-personpickers |
Merge into: | lp:launchpad |
Diff against target: |
172 lines (+67/-31) 3 files modified
lib/lp/app/javascript/picker.js (+11/-15) lib/lp/app/javascript/tests/test_personpicker.js (+18/-2) lib/lp/app/javascript/widgets.js (+38/-14) |
To merge this branch: | bzr merge lp:~jcsackett/launchpad/picker-patcher-picked-a-patch-of-personpickers |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Curtis Hovey (community) | code | Approve | |
Ian Booth | Pending | ||
Review via email: mp+64094@code.launchpad.net |
Commit message
[r=sinzui][bug=796635] Updates addPickerPatcher to use the personpicker.
Description of the change
Summary
=======
After creating the new js personpicker widget, we realized that there was an alternative means of getting a person picker via lp.app.
Proposed fix
============
Set up addPickerPatcher to use a personpicker to create the "Assign me" and "Remove assignee" buttons as it needs them, since this is one of the major things added in the person picker, and we would like to avoid duplicate javascript.
Pre-implementation notes
=======
Spoke with Curtis Hovey and Ian Booth about the duplication and the idea of simply using personpicker to create the assignme and remove buttons in picker patcher.
Implementation details
=======
lib/lp/
-------
Updated PersonPicker to generate the extra buttons based on the presence of show_assign_me and show_remove config options, so it can use the information passed along when the addPickerPatcher is used.
Also, added the remove and assign buttons as properties on the picker so that addPickerPatcher could easily override the functions attached to those buttons click events.
lib/lp/
-------
Removed the code generating the extra buttons in addPickerPatcher, since that's now done by the personpicker. Retained the custom assign_me and remove methods, which are now added to the personpicker's assign/remove buttons after purgeElement is called to remove the personpicker's usual events when those buttons are clicked.
Tests
=====
firefox lib/lp/
firefox lib/lp/
Demo and Q/A
============
Check the various pickers; they should all function the same as prior to this branch landing. This is just cleaning up code.
Launchpad lint
==============
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/
lib/lp/
./lib/lp/
14: 'Picker' has not been fully defined yet.
62: 'PersonPicker' has not been fully defined yet.
I tried changing the lines causing this to use 'this' instead of 'Picker' or 'PersonPicker', but that broke the code. Perhaps this is an issue in the linter?
I think line 82 should use !==, not !=: remove_ button !== undefined) {
if (cfg.show_
This is good to land with this fix.