Fine tune the charm token drag and drop.

Fine tune the charm token drag and drop.

- change the drag token to be the charm icon
- make the service ghost appear where the user dropped the charm icon
- fix browser wierdness around dragImage elements
- tweak the way the tests set the charm token container

https://code.launchpad.net/~benji/juju-gui/charm-token-dnd-tweaks/+merge/172139

(do not edit description out of merge proposal)

[benji, 2013-06-28 19:46:07]

Please take a look.

[matthew.scott, 2013-06-28 19:57:34]

LGTM - thanks for the branch!

[jeff.pihach, 2013-06-28 20:14:03]

LGTM Very....cool!

Can you create a quick follow-up card to add this functionality to the new
deploy method that's now in trunk in ghost-inspector.js. Doing this will solve
the following two issues:
If you drag or click to deploy you get two different configuration panels.
If you drag to deploy the inspector panel is broken (no settings config)

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js#new...
app/widgets/charm-token.js:52: delete this.cfg.contentBox;
This is a very fragile approach to this issue.

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js#new...
app/widgets/charm-token.js:95: .setStyle('height',
icon.one('img').get('height'))
small performance request here - use setStyles({}) and make one query request
for the image and cache it.

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js#new...
app/widgets/charm-token.js:105: evt.dataTransfer.effectAllowed = 'copy';
just an FYI if you assign evt.dataTransfer to another var it will minify better.

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js#new...
app/widgets/charm-token.js:147: // We need all the children to participate.
this is so odd.

https://codereview.appspot.com/10763043/diff/1/lib/views/browser/charm-token....
File lib/views/browser/charm-token.less (right):

https://codereview.appspot.com/10763043/diff/1/lib/views/browser/charm-token....
lib/views/browser/charm-token.less:31: z-index: 10;
/* magic number warning */ ;-)

[benji, 2013-06-28 20:17:48]

Please take a look.

[benji, 2013-06-28 20:33:46]

Thanks guys for the very good reviews.  In light of our desire to get a release
made that contains this functionality, I'm going to address the review comments
in a follow-on branch.

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js
File app/widgets/charm-token.js (right):

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js#new...
app/widgets/charm-token.js:52: delete this.cfg.contentBox;
On 2013/06/28 20:14:03, jeff.pihach wrote:
> This is a very fragile approach to this issue. 

Indeed.  I'll see if I can address this quickly in the follow-on.

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js#new...
app/widgets/charm-token.js:95: .setStyle('height',
icon.one('img').get('height'))
On 2013/06/28 20:14:03, jeff.pihach wrote:
> small performance request here - use setStyles({}) and make one query request
> for the image and cache it.

That feels like a premature optimization, but I guess it won't kill me.

https://codereview.appspot.com/10763043/diff/1/app/widgets/charm-token.js#new...
app/widgets/charm-token.js:105: evt.dataTransfer.effectAllowed = 'copy';
On 2013/06/28 20:14:03, jeff.pihach wrote:
> just an FYI if you assign evt.dataTransfer to another var it will minify
better.

I'd rather not introduce a level of indirection for the sake of a small
minimization gain.

https://codereview.appspot.com/10763043/diff/1/lib/views/browser/charm-token....
File lib/views/browser/charm-token.less (right):

https://codereview.appspot.com/10763043/diff/1/lib/views/browser/charm-token....
lib/views/browser/charm-token.less:31: z-index: 10;
On 2013/06/28 20:14:03, jeff.pihach wrote:
> /* magic number warning */ ;-)

Yeah, I'm not happy with that.  I'll see if I can remove it.

[benji, 2013-06-28 20:44:43]

*** Submitted:

Fine tune the charm token drag and drop.

- change the drag token to be the charm icon
- make the service ghost appear where the user dropped the charm icon
- fix browser wierdness around dragImage elements
- tweak the way the tests set the charm token container

R=matthew.scott, jeff.pihach
CC=
https://codereview.appspot.com/10763043