Creates the basic sharing widget

Creates the basic sharing widget

-Sharing moved behind feature flag while it's ugly and partially functional
-Sharing widget with images and basic layout
-Share button shows/hides the widget

https://code.launchpad.net/~jcsackett/juju-gui/sharing-widget/+merge/170615

(do not edit description out of merge proposal)

[j.c.sackett, 2013-06-20 13:34:50]

Please take a look.

[rharding, 2013-06-20 13:57:39]

Feedback below.

https://codereview.appspot.com/10352045/diff/1/app/subapps/browser/views/char...
File app/subapps/browser/views/charm.js (right):

https://codereview.appspot.com/10352045/diff/1/app/subapps/browser/views/char...
app/subapps/browser/views/charm.js:510: tplData.shareFlag = false;
What would be awesome would be to add a handlebars extension to check flags
instead of passing to the template.

https://codereview.appspot.com/10352045/diff/1/app/templates/sharing-overlay....
File app/templates/sharing-overlay.handlebars (right):

https://codereview.appspot.com/10352045/diff/1/app/templates/sharing-overlay....
app/templates/sharing-overlay.handlebars:4: <img
src="/juju-ui/assets/images/facebook-normal.png"/>
shouldn't this use the sprite css classes?

https://codereview.appspot.com/10352045/diff/1/lib/views/browser/sharing-over...
File lib/views/browser/sharing-overlay.less (right):

https://codereview.appspot.com/10352045/diff/1/lib/views/browser/sharing-over...
lib/views/browser/sharing-overlay.less:7: border: solid 1px #dddddd;
can you use the css vars setup in the vars.less? Add the background maybe if
there's a good name for it?

[j.c.sackett, 2013-06-20 14:01:01]

On 2013/06/20 13:57:39, rharding wrote:
> Feedback below.
> 
>
https://codereview.appspot.com/10352045/diff/1/app/subapps/browser/views/char...
> File app/subapps/browser/views/charm.js (right):
> 
>
https://codereview.appspot.com/10352045/diff/1/app/subapps/browser/views/char...
> app/subapps/browser/views/charm.js:510: tplData.shareFlag = false;
> What would be awesome would be to add a handlebars extension to check flags
> instead of passing to the template.

Perhaps, but that feels out of scope for this branch, particularly as this flag
should ideally not live past next week.

>
https://codereview.appspot.com/10352045/diff/1/app/templates/sharing-overlay....
> File app/templates/sharing-overlay.handlebars (right):
> 
>
https://codereview.appspot.com/10352045/diff/1/app/templates/sharing-overlay....
> app/templates/sharing-overlay.handlebars:4: <img
> src="/juju-ui/assets/images/facebook-normal.png"/>
> shouldn't this use the sprite css classes?

Should it? I see lots of cases where we're linking via image. Also, don't
anchors without an actual element inside cause issues on some browsers (e.g.
IE?)

>
https://codereview.appspot.com/10352045/diff/1/lib/views/browser/sharing-over...
> File lib/views/browser/sharing-overlay.less (right):
> 
>
https://codereview.appspot.com/10352045/diff/1/lib/views/browser/sharing-over...
> lib/views/browser/sharing-overlay.less:7: border: solid 1px #dddddd;
> can you use the css vars setup in the vars.less? Add the background maybe if
> there's a good name for it?

Can you use those variables if they're not defined within the less file? If so,
sure, otherwise I don't see the value in redefining them here.

[rharding, 2013-06-20 14:13:34]

On 2013/06/20 14:01:01, j.c.sackett wrote:
> On 2013/06/20 13:57:39, rharding wrote:
> > Feedback below.
> > 
> >
>
https://codereview.appspot.com/10352045/diff/1/app/subapps/browser/views/char...
> > File app/subapps/browser/views/charm.js (right):
> > 
> >
>
https://codereview.appspot.com/10352045/diff/1/app/subapps/browser/views/char...
> > app/subapps/browser/views/charm.js:510: tplData.shareFlag = false;
> > What would be awesome would be to add a handlebars extension to check flags
> > instead of passing to the template.
> 
> Perhaps, but that feels out of scope for this branch, particularly as this
flag
> should ideally not live past next week.

Yes, but it's adding something handy to the base code while we're in here. It's
a few lines of code, but I understand that this is short-lived and will LGTM
without it. 

> 
> >
>
https://codereview.appspot.com/10352045/diff/1/app/templates/sharing-overlay....
> > File app/templates/sharing-overlay.handlebars (right):
> > 
> >
>
https://codereview.appspot.com/10352045/diff/1/app/templates/sharing-overlay....
> > app/templates/sharing-overlay.handlebars:4: <img
> > src="/juju-ui/assets/images/facebook-normal.png"/>
> > shouldn't this use the sprite css classes?
> 
> Should it? I see lots of cases where we're linking via image. Also, don't
> anchors without an actual element inside cause issues on some browsers (e.g.
> IE?)

Yes, we should use the sprites since we've got them. Please check if it causes
any issues in IE10. It's the only IE we're caring about. Especially with the
hover/etc using sprites will make things load faster and more responsive to
users.

> 
> >
>
https://codereview.appspot.com/10352045/diff/1/lib/views/browser/sharing-over...
> > File lib/views/browser/sharing-overlay.less (right):
> > 
> >
>
https://codereview.appspot.com/10352045/diff/1/lib/views/browser/sharing-over...
> > lib/views/browser/sharing-overlay.less:7: border: solid 1px #dddddd;
> > can you use the css vars setup in the vars.less? Add the background maybe if
> > there's a good name for it?
> 
> Can you use those variables if they're not defined within the less file? If
so,
> sure, otherwise I don't see the value in redefining them here.

Yes, the point is to keep all css variables in one place and reuse them in the
various files. In this one, an update to say, border color, will cascade across
the project in one edit. Since all the files are concatenated, the rules are in
the file already for you to use. You don't have to define them. Always try to
make the css as reusable as possible with the less tool as we can.

[jeff.pihach, 2013-06-20 14:24:38]

LGTM with some trivial comments. I'm assuming that this is still very much a WIP

https://codereview.appspot.com/10352045/diff/1/app/widgets/sharing-overlay.js
File app/widgets/sharing-overlay.js (right):

https://codereview.appspot.com/10352045/diff/1/app/widgets/sharing-overlay.js...
app/widgets/sharing-overlay.js:31: ns.SharingOverlay =
Y.Base.create('SharingOverlay', Y.Widget, [], {
It's a little odd that this is called an overlay but it's just a widget.

https://codereview.appspot.com/10352045/diff/1/app/widgets/sharing-overlay.js...
app/widgets/sharing-overlay.js:54: this.get('button').on('click',
this._toggleVisible, this);
not being tracked for detach on destroy

[j.c.sackett, 2013-06-20 14:29:59]

On 2013/06/20 14:24:38, jeff.pihach wrote:
> LGTM with some trivial comments. I'm assuming that this is still very much a
WIP

It is, thus being landed behind a flag. Still, anything you see as a potential
problem should be pointed out. I can fix it in the follow up branches and would
rather know now.

> https://codereview.appspot.com/10352045/diff/1/app/widgets/sharing-overlay.js
> File app/widgets/sharing-overlay.js (right):
> 
>
https://codereview.appspot.com/10352045/diff/1/app/widgets/sharing-overlay.js...
> app/widgets/sharing-overlay.js:31: ns.SharingOverlay =
> Y.Base.create('SharingOverlay', Y.Widget, [], {
> It's a little odd that this is called an overlay but it's just a widget.

Fair point; it got called sharing overlay a couple of times when talking about
it, and I kept that name, but I'm down with renaming it.

>
https://codereview.appspot.com/10352045/diff/1/app/widgets/sharing-overlay.js...
> app/widgets/sharing-overlay.js:54: this.get('button').on('click',
> this._toggleVisible, this);
> not being tracked for detach on destroy

I'll fix this.

[rharding, 2013-06-20 15:59:31]

LGTM with the changes to using the less variables and the spriting of the images
as long as it's IE10 safe. If not then we can look at improvements when we get
to adding the hover and such.

[j.c.sackett, 2013-06-20 16:06:15]

On 2013/06/20 14:13:34, rharding wrote:
> Yes, but it's adding something handy to the base code while we're in here.
It's
> a few lines of code, but I understand that this is short-lived and will LGTM
> without it.

Fair cop; since I mostly want this landed so design can start looking at it
ASAP, I think it's still out of scope for this, but I've got a pipe now to do
this that I'll put up soon.

> Yes, we should use the sprites since we've got them. Please check if it causes
> any issues in IE10. It's the only IE we're caring about. Especially with the
> hover/etc using sprites will make things load faster and more responsive to
> users.

Ok, I wasn't sure; there are many places that don't, but I guess they just
haven't been updated. Doing this.

> Yes, the point is to keep all css variables in one place and reuse them in the
> various files. In this one, an update to say, border color, will cascade
across
> the project in one edit. Since all the files are concatenated, the rules are
in
> the file already for you to use. You don't have to define them. Always try to
> make the css as reusable as possible with the less tool as we can.

Dude, I know the *point* of it. :-P 

I thought there was a technical limitation in that included less files didn't
get access. I'm updating the less file.

[j.c.sackett, 2013-06-20 18:48:28]

Please take a look.

[j.c.sackett, 2013-06-20 22:24:15]

*** Submitted:

Creates the basic sharing widget

-Sharing moved behind feature flag while it's ugly and partially functional
-Sharing widget with images and basic layout
-Share button shows/hides the widget

R=rharding, jeff.pihach
CC=
https://codereview.appspot.com/10352045