Code review comment for lp:~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2

On Mon, 2009-12-21 at 20:54 +0000, Edwin Grubbs wrote:
> Here is the incremental diff that addresses both Salgado's and
> Martin's comments.

Looks good to me, but there's some code that was apparently written to
test corner cases and left behind accidentally. See below

>
> {{{
> === modified file 'lib/canonical/launchpad/javascript/lp/errors.js'
> --- lib/canonical/launchpad/javascript/lp/errors.js 2009-11-23 19:29:02 +0000
> +++ lib/canonical/launchpad/javascript/lp/errors.js 2009-12-21 20:33:38 +0000
> @@ -69,7 +69,7 @@
> * @param flash_node {Node} The node to red flash.
> * @param msg {String} The message to display.
> */
> -var display_error = function(flash_node, msg) {
> +Y.lp.display_error = function(flash_node, msg) {
> create_error_overlay();
> maybe_red_flash(flash_node, function(){
> error_overlay.showError(msg);
> @@ -77,5 +77,36 @@
> });
> };
>
> -Y.lp.display_error = display_error;
> -}, "0.1", {"requires":["lazr.formoverlay"]});
> +
> +var info_overlay;
> +/*
> + * Display the form overlay for non-error informational messages.
> + *
> + * @method display_info
> + * @param msg {String} The message to display.
> +*/
> +Y.lp.display_info = function(msg) {
> + if (info_overlay === undefined) {
> + info_overlay = new Y.lazr.PrettyOverlay({
> + //headerContent: '<h2><img src="/@@/info-large"/>Notice</h2>',

Why is this commented out?

> + centered: true,
> + visible: false
> + });
> + info_overlay.render();
> + }
> + var content = Y.Node.create(
> + '<div style="background: url(/@@/info-large) no-repeat; ' +
> + 'min-height: 32px; padding-left: 40px; padding-top: 16px"/></div>');
> + content.appendChild(Y.Node.create(msg));
> + var button_div = Y.Node.create('<div style="text-align: right"></div>');
> + var ok_button = Y.Node.create('<button>OK</button>');
> + ok_button.on('click', function(e) {
> + info_overlay.fire('cancel');
> + });
> + button_div.appendChild(ok_button);
> + content.appendChild(button_div);
> + info_overlay.set('bodyContent', content);
> + info_overlay.show();
> +};
> +
> +}, "0.1", {"requires":["lazr.formoverlay", "lazr.overlay"]});
>
> === modified file 'lib/canonical/launchpad/javascript/registry/team.js'
> --- lib/canonical/launchpad/javascript/registry/team.js 2009-12-18 06:06:30 +0000
> +++ lib/canonical/launchpad/javascript/registry/team.js 2009-12-21 20:35:57 +0000
> @@ -54,13 +54,14 @@
> var members_section, members_ul, count_elem;
> if (did_status_change === false) {
> disable_spinner();
> - Y.lp.display_error(
> - addmember_link,
> + Y.lp.display_info(
> selected_person.title + ' is already ' +
> - current_status + '.');
> + current_status.toLowerCase() +
> + ' as a member of the team.');
> return;
> }
> var is_team = false;
> + current_status = 'foo';

!?

I really hope your windmill test fails with this change...

> if (current_status == 'Invited') {
> is_team = true;
> members_section = box.query('#recently-invited');
> @@ -79,6 +80,7 @@
> Y.lp.display_error(
> addmember_link,
> 'Unexpected status: ' + current_status);
> + return;
> }
> var first_node = members_ul.get('firstChild');
>

--
Guilherme Salgado <email address hidden>