Add support for loading hooks file content.

Add support for loading hooks file content.

- Render out the list of files in the hooks tab
- Bind those to a click handler that loads the file's content
- Add test for these
- Start to wire up OverlayIndicator to note 'loading'
- Fix bugs in OverlayIndicator for image path
- Adjust CSS build to load OverlayIndicator rules
- Start to use yui3-g-r for some layout.

https://code.launchpad.net/~rharding/juju-gui/charm_hooks/+merge/156204

(do not edit description out of merge proposal)

[rharding, 2013-03-29 18:34:55]

Please take a look.

[matthew.scott, 2013-03-29 19:06:33]

LGTM - thanks for the branch.

The code looks good, and runs well (though I'm not totally up on the story). 
From what I understand, styling will come later, but just as a note for now, the
cursor for the links to the hooks is still a text selection cursor, and should
probably be a 'pointer' cursor when it comes time to style.  Also, I think I
remember that charms are moving to markdown, but for those still using reST, is
there a similar YUI plugin?  Just a question.

[curtis, 2013-03-29 19:19:08]

Hi Rick. This implementation looks good, but I wonder if my comment about 'thi'
below indicates our tests are not good enough.

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm.js
File app/subapps/browser/views/charm.js (left):

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:92: );
'thi' looks wrong. I think you mean 'this'.

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

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:106: } else if (filename.slice(-3) === '.md')
{
Maybe we want to close this bug:
    https://bugs.launchpad.net/charmworld/+bug/988489
[switch out highlighting to highlight.js] because I don't think we are happy.
What do you think?

[j.c.sackett, 2013-03-29 19:27:11]

LGTM, just one comment.

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

https://codereview.appspot.com/8166043/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:56: img_url);
We should update the img_url code to

`var img_url = img.get('src').split('/').slice(3).join('/')`

and then

`assert.equal(
    'juju-ui/assets/images/non-sprites/loading-spinner.gif',
    img_url);`

[gary.poster, 2013-03-29 19:44:13]

I was going to review this, but then I saw you had a bunch already. :-) I did
keep the two comments I had made so far, though...

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

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:108: data = Y.Markdown.toHTML(data);
You never actually put this data in the container, do you?

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:226: * Show/setBusy an indicator for a given
node. If it already exists, then
I can't parse that first sentence.

[curtis, 2013-03-29 19:51:38]

On 2013/03/29 19:19:08, curtis wrote:
> Hi Rick. This implementation looks good, but I wonder if my comment about
'thi'
> below indicates our tests are not good enough.

NM. RV was not showing the 's' in this in my browser. It is there in the code.
LGTM.

[rharding, 2013-03-29 19:55:20]

On 2013/03/29 19:06:33, matthew.scott wrote:
> LGTM - thanks for the branch.
> 
> The code looks good, and runs well (though I'm not totally up on the story). 
> From what I understand, styling will come later, but just as a note for now,
the
> cursor for the links to the hooks is still a text selection cursor, and should
> probably be a 'pointer' cursor when it comes time to style.  Also, I think I
> remember that charms are moving to markdown, but for those still using reST,
is
> there a similar YUI plugin?  Just a question.

Thanks, yes the design is all coming later. This is to setup the framework for
design to follow. Actually, I think all the files will not be <a> tags once
design hits, but for now it helps point out what's 'clickable'. 

As for the .rst, I had the same thought but it doesn't appear we've got anything
handy so I'm treating it normally for now. We can adjust the parser later if we
do find one. It's a 'nice to have' at this point.

[rharding, 2013-03-29 19:55:40]

Comment replies before. Will push updates in a bit.

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

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:106: } else if (filename.slice(-3) === '.md')
{
On 2013/03/29 19:19:08, curtis wrote:
> Maybe we want to close this bug:
>     https://bugs.launchpad.net/charmworld/+bug/988489
> [switch out highlighting to highlight.js] because I don't think we are happy.
> What do you think?

I've not compared the implementations or seen the issue. So I guess we can leave
the bug open and look to replace prettify if we want, but we've got prettify for
now.

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:108: data = Y.Markdown.toHTML(data);
On 2013/03/29 19:44:13, gary.poster wrote:
> You never actually put this data in the container, do you?

Ah, good point. I don't actually have a test that parses markdown. I'll add one
and it would have caught this refactoring bug from before.

https://codereview.appspot.com/8166043/diff/1/app/subapps/browser/views/charm...
app/subapps/browser/views/charm.js:226: * Show/setBusy an indicator for a given
node. If it already exists, then
On 2013/03/29 19:44:13, gary.poster wrote:
> I can't parse that first sentence.

I'll reword. The issue is that there's one div for displaying the contents of
files. It is hit over and over again when you click on many files in a row. This
helps make sure that we don't re-create an overlay widget when we've already got
one targeted to this node.

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

https://codereview.appspot.com/8166043/diff/1/test/test_overlay_indicator.js#...
test/test_overlay_indicator.js:56: img_url);
On 2013/03/29 19:27:11, j.c.sackett wrote:
> We should update the img_url code to
> 
> `var img_url = img.get('src').split('/').slice(3).join('/')`
> 
> and then
> 
> `assert.equal(
>     'juju-ui/assets/images/non-sprites/loading-spinner.gif',
>     img_url);`

Ah ok. Will do.

[rharding, 2013-03-29 20:20:08]

Please take a look.

[rharding, 2013-03-29 20:30:48]

*** Submitted:

Add support for loading hooks file content.

- Render out the list of files in the hooks tab
- Bind those to a click handler that loads the file's content
- Add test for these
- Start to wire up OverlayIndicator to note 'loading'
- Fix bugs in OverlayIndicator for image path
- Adjust CSS build to load OverlayIndicator rules
- Start to use yui3-g-r for some layout.

R=matthew.scott, curtis, j.c.sackett, gary.poster
CC=
https://codereview.appspot.com/8166043