Charm deploy from front page.

Charm deploy from front page.

Make charm deployment possible.  Also fixed bug 1053127 as a drive-by.
Note use of ev.stopPropagation() in showCharm to prevent the URL from
changing.

https://code.launchpad.net/~bac/juju-ui/charm-deploy/+merge/127781

(do not edit description out of merge proposal)

[bac, 2012-10-03 14:32:21]

Please take a look.

[bac, 2012-10-03 14:42:04]

Please take a look.

[hazmat, 2012-10-03 15:52:50]

a couple of issues, i can't seem to close the charm search dialog. The clicking
on the charms text or chevron won't reverse it. Attempting to click through to
details of a given charm generally doesn't work, there appears to be an
additional 'charms' prefix added. The search series grouping will put
pre-release software first in the list (quantal for example), we can change the
backend formatting here if we need to.

https://codereview.appspot.com/6601045/diff/2001/app/templates/charm-search-r...
File app/templates/charm-search-result-entries.handlebars (right):

https://codereview.appspot.com/6601045/diff/2001/app/templates/charm-search-r...
app/templates/charm-search-result-entries.handlebars:11: data-url="{{url}}"
why are we repeating the same thing formatted differently multiple times?

[bac, 2012-10-03 21:10:43]

On 2012/10/03 15:52:50, hazmat wrote:
> a couple of issues, i can't seem to close the charm search dialog. The
clicking
> on the charms text or chevron won't reverse it. Attempting to click through to
> details of a given charm generally doesn't work, there appears to be an
> additional 'charms' prefix added. 

Both of these issues were caused by merge problems after the branch was ready to
submit.  The merge looked clean enough so it wasn't thoroughly tested again
before sending it on.

> The search series grouping will put
> pre-release software first in the list (quantal for example), we can change
the
> backend formatting here if we need to.

I think the backend will need to change if we want to change the series sort
order.  How would the client distinguish between the released and unreleased
series?

[bac, 2012-10-03 21:10:51]

https://codereview.appspot.com/6601045/diff/2001/app/templates/charm-search-r...
File app/templates/charm-search-result-entries.handlebars (right):

https://codereview.appspot.com/6601045/diff/2001/app/templates/charm-search-r...
app/templates/charm-search-result-entries.handlebars:11: data-url="{{url}}"
It is confusing due to the convention of using the 'data-' prefix and YUI
getData() without the prefix to retrieve them.

data-url is the URL for the charm.
data-name is the name
data-info-url is the local URL for getting the details of the charm.  All of
those are passed via the Deploy button.

The last is also shown in the link.

So there really is no redundancy.

[bac, 2012-10-03 21:15:55]

Please take a look.

[bac, 2012-10-04 15:35:08]

Please take a look.

[hazmat, 2012-10-04 21:48:22]

https://codereview.appspot.com/6601045/diff/8002/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6601045/diff/8002/app/views/charm-search.js#ne...
app/views/charm-search.js:171: 
This code feels a bit dodgy given the built-in app support for persistent views
(ie. what notifications do). The panel display then falls out as a the
persistent container view with management around the active subview.

Given time constraints i'm fine with accumulating it as technical debt, but
please make a card about cleaning this up, as a self-contained example of the
alternative, see 
http://jsbin.com/ojalij/1/edit

As it is the charm collection view gets created and rendered on every
navigation.

magic testing parameterization/hardcode is also questionable, versus, just being
able to have the tests pass in the parameters to achieve the desired behavior
ie. searchDelay 0.

Also rather than managing an isPopupVisible variable, a yui3 visible() test
against the container would suffice.

[gary.poster, 2012-10-04 22:42:24]

Quick replies to hazmat.

https://codereview.appspot.com/6601045/diff/8002/app/views/charm-search.js
File app/views/charm-search.js (right):

https://codereview.appspot.com/6601045/diff/8002/app/views/charm-search.js#ne...
app/views/charm-search.js:171: 
On 2012/10/04 21:48:22, hazmat wrote:
> This code feels a bit dodgy given the built-in app support for persistent
views
> (ie. what notifications do). The panel display then falls out as a the
> persistent container view with management around the active subview.

While agree completely that this code should be refactored, and generally agree
that sticking with YUI component bits would be better, I'm not sure that the
app-in-app approach is actually easier to read.  That said, the infrastructure
advantages make it a win, I now agree.

> Given time constraints i'm fine with accumulating it as technical debt, but
> please make a card about cleaning this up, as a self-contained example of the
> alternative, see 
> http://jsbin.com/ojalij/1/edit

+1, though the refactoring may fall out of other work, just as the separation of
the sub-view fell out.

> As it is the charm collection view gets created and rendered on every
> navigation.

That's actually not true even in the current approach. 
views.CharmSearchPopup.getInstance makes a singleton, so the variables are made
once per top-level page load.

> magic testing parameterization/hardcode is also questionable, versus, just
being
> able to have the tests pass in the parameters to achieve the desired behavior
> ie. searchDelay 0.

Hm.  If the only point of a particular configuration is to make something
testable, why does it need to be exposed?  I grant it is questionable, but I
also don't think that the answer is a given.

> 
> Also rather than managing an isPopupVisible variable, a yui3 visible() test
> against the container would suffice.

Agreed.