Ports the lp overlay indicator for charm browser

Ports the lp overlay indicator for charm browser

This ports the overlay indicator widget from lp for use in the charm browser
subapp.

The overlay indicator is a "loading" widget that can temporarily cover any DOM
element; provided an element at creation it sizes itself to that element and
displays a loading spinner until it is told that the covered element is ready.

https://code.launchpad.net/~jcsackett/juju-gui/indicator-widget/+merge/155075

(do not edit description out of merge proposal)

[j.c.sackett, 2013-03-22 21:57:45]

Please take a look.

[jeff.pihach, 2013-03-22 22:33:42]

Thanks for this - lots of tests!

Most of the comments below are trivial but I would like you to look at the
change to test-server.sh and to destroy the instances in your tests.

Have a great weekend.

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

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:43: var local_parent =
this.get('target').get('parentNode');
localParent

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:56: img.set('src',
'/juju-ui/assets/images/loading-spinner.gif');
Is there a reason why you don't set the src in the template? and then just place
the string inside the create parens?

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:92: setBusy: function() {
Is this method going to have any extra functionality besides this.show() ? If
not I don't really see the benefit of creating a level of abstraction when
widget.show() could be called following the convention.

If there will be extra stuff that could be handled by
this.on('visibleChange', /*do setBusy stuff*/);

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:117: if (Y.Lang.isFunction(callback)) {
micro optimization: typeof callback == 'function'

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:131: value: null
just a friendly reminder that null is special in js so unless there is a reason
why target: {} (which would make value undefined) isn't good enough I'd probably
stick with undefined.

https://codereview.appspot.com/7792045/diff/1/lib/views/browser/overlay-indic...
File lib/views/browser/overlay-indicator.less (right):

https://codereview.appspot.com/7792045/diff/1/lib/views/browser/overlay-indic...
lib/views/browser/overlay-indicator.less:3: /* for css3 browsers we can limit
opacity via alpha channel */
Do we support any browsers that don't have rgba?

https://codereview.appspot.com/7792045/diff/1/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/7792045/diff/1/test-server.sh#newcode8
test-server.sh:8: xdg-open http://0.0.0.0:8084/test/index.html
Was this committed intentionally or was this for your own machine? Please revert
back to localhost if it was for your own machine. 

It would actually cause this script to fail on my machine :-)

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js
File test/test_overlay_indicator.js (right):

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:22: it('exists', function() {
I'm not entirely sure this test is necessary because the following test will
fail if it doesn't exist :-)

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:28: var indicator = new
Y.juju.browser.widgets.OverlayIndicator(
You create a new instance of this every test but never destroy it - if this has
event listeners attached then it will leave them dangling in the test runner.

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:66: it('shows on setBusy', function() {
see my comment about setBusy() in the widget

[rharding, 2013-03-25 12:03:07]

lgtm as long as we catch events and detach them along with destroying the widget
instance in tests. 

I'd setup a common var name `instance` or something and then in the teardown
just do a if (instance) instance.destroy();

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

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:92: setBusy: function() {
On 2013/03/22 22:33:42, jeff.pihach wrote:
> Is this method going to have any extra functionality besides this.show() ? If
> not I don't really see the benefit of creating a level of abstraction when
> widget.show() could be called following the convention.
> 
> If there will be extra stuff that could be handled by
> this.on('visibleChange', /*do setBusy stuff*/);

This is a widget pulled from another project. The idea was to provide a nice api
calls you'd expect. setBusy was determined to be a clear 'show' method name.
Notice that we don't require srcNode/parentNode/etc. It's target which is the
node we want to match/overlay on top of.

https://codereview.appspot.com/7792045/diff/1/lib/views/browser/overlay-indic...
File lib/views/browser/overlay-indicator.less (right):

https://codereview.appspot.com/7792045/diff/1/lib/views/browser/overlay-indic...
lib/views/browser/overlay-indicator.less:3: /* for css3 browsers we can limit
opacity via alpha channel */
On 2013/03/22 22:33:42, jeff.pihach wrote:
> Do we support any browsers that don't have rgba?

The first line that just uses a white background is fallback for non-rgba
browsers.

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js
File test/test_overlay_indicator.js (right):

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:22: it('exists', function() {
On 2013/03/22 22:33:42, jeff.pihach wrote:
> I'm not entirely sure this test is necessary because the following test will
> fail if it doesn't exist :-)

This is just usually the first test we write for a new module to make sure the
files are loading/etc correctly. It can be removed, but is usually just left to
fail noting that the module's got issues beyond possible conditions in the
following test cases.

[j.c.sackett, 2013-03-25 16:17:48]

Please take a look.

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

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:43: var local_parent =
this.get('target').get('parentNode');
On 2013/03/22 22:33:42, jeff.pihach wrote:
> localParent

Got it.

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:56: img.set('src',
'/juju-ui/assets/images/loading-spinner.gif');
On 2013/03/22 22:33:42, jeff.pihach wrote:
> Is there a reason why you don't set the src in the template? and then just
place
> the string inside the create parens? 

Nothing good; I was focused on porting this over and making sure it worked and
passed the linter, and reviewing for obvious bad choices. Things like this
didn't pop up in my work, but I can make that change.

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:92: setBusy: function() {
On 2013/03/22 22:33:42, jeff.pihach wrote:
> Is this method going to have any extra functionality besides this.show() ? If
> not I don't really see the benefit of creating a level of abstraction when
> widget.show() could be called following the convention.
> 
> If there will be extra stuff that could be handled by
> this.on('visibleChange', /*do setBusy stuff*/);

As Rick said, this widget was pulled from another project, but its design
choices here make sense. We're not rendering this in quite the same way as a
standard widget--when this is rendered, its explicitly to set another
action/widget as "busy"--this name makes that clear when browsing through code
using it.

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:117: if (Y.Lang.isFunction(callback)) {
On 2013/03/22 22:33:42, jeff.pihach wrote:
> micro optimization: typeof callback == 'function'

Ok.

https://codereview.appspot.com/7792045/diff/1/app/widgets/overlay-indicator.j...
app/widgets/overlay-indicator.js:131: value: null
On 2013/03/22 22:33:42, jeff.pihach wrote:
> just a friendly reminder that null is special in js so unless there is a
reason
> why target: {} (which would make value undefined) isn't good enough I'd
probably
> stick with undefined.

Really? I thought null was better, since it usually meant something was
specifically marked as not yet existing, whereas there are many ways something
might become undefined.

https://codereview.appspot.com/7792045/diff/1/lib/views/browser/overlay-indic...
File lib/views/browser/overlay-indicator.less (right):

https://codereview.appspot.com/7792045/diff/1/lib/views/browser/overlay-indic...
lib/views/browser/overlay-indicator.less:3: /* for css3 browsers we can limit
opacity via alpha channel */
On 2013/03/22 22:33:42, jeff.pihach wrote:
> Do we support any browsers that don't have rgba?

I don't know; since someone had already done the work in this widget's CSS of
supporting it, I didn't think it was a good idea to deliberately remove it when
we might.

https://codereview.appspot.com/7792045/diff/1/test-server.sh
File test-server.sh (right):

https://codereview.appspot.com/7792045/diff/1/test-server.sh#newcode8
test-server.sh:8: xdg-open http://0.0.0.0:8084/test/index.html
On 2013/03/22 22:33:42, jeff.pihach wrote:
> Was this committed intentionally or was this for your own machine? Please
revert
> back to localhost if it was for your own machine. 
> 
> It would actually cause this script to fail on my machine :-)

Totally accidental; I run this in an LXC b/c I don't want all the dependencies
on my main machine, but it means I have to hack this so I can check things in my
browser. Usually I remember to revert it/not commit it. 

I've already reverted it locally and will push that up with the other changes.

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js
File test/test_overlay_indicator.js (right):

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:22: it('exists', function() {
On 2013/03/22 22:33:42, jeff.pihach wrote:
> I'm not entirely sure this test is necessary because the following test will
> fail if it doesn't exist :-)

As Rick says, I like having a test like this because if it fails along with
anything else, you know it's because the module/object isn't being pulled in.

If you want me to remove a test, I would actually suggest the next one, I would
suggest "gets target from config" because widgets getting ATTRs from cfg is well
tested in YUI.

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:28: var indicator = new
Y.juju.browser.widgets.OverlayIndicator(
On 2013/03/22 22:33:42, jeff.pihach wrote:
> You create a new instance of this every test but never destroy it - if this
has
> event listeners attached then it will leave them dangling in the test runner.

I'm addressing this per Rick's suggestion for refactoring the tests.

https://codereview.appspot.com/7792045/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:66: it('shows on setBusy', function() {
On 2013/03/22 22:33:42, jeff.pihach wrote:
> see my comment about setBusy() in the widget

See notes from Rick and I about why it's setBusy

[j.c.sackett, 2013-03-25 16:18:30]

Response to comments here, code coming.

[j.c.sackett, 2013-03-25 16:19:51]

On 2013/03/25 16:18:30, j.c.sackett wrote:
> Response to comments here, code coming.

Or the code came first, so the comments are in the previous patch set.

[j.c.sackett, 2013-03-25 19:16:22]

Please take a look.

[jeff.pihach, 2013-03-25 19:27:44]

LGTM Thanks for those changes - couple attribute documents need to be updated to
default undefined from null.

https://codereview.appspot.com/7792045/diff/19001/app/widgets/overlay-indicat...
File app/widgets/overlay-indicator.js (right):

https://codereview.appspot.com/7792045/diff/19001/app/widgets/overlay-indicat...
app/widgets/overlay-indicator.js:139: * @default null
default is undefined

https://codereview.appspot.com/7792045/diff/19001/app/widgets/overlay-indicat...
app/widgets/overlay-indicator.js:148: * @default null
default is undefined

[j.c.sackett, 2013-03-25 19:36:10]

*** Submitted:

Ports the lp overlay indicator for charm browser

This ports the overlay indicator widget from lp for use in the charm browser
subapp.

The overlay indicator is a "loading" widget that can temporarily cover any DOM
element; provided an element at creation it sizes itself to that element and
displays a loading spinner until it is told that the covered element is ready.

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