Adds handling to category links

Adds handling to category links

-Filter model now has a reset mechanism
-Filter model's reset is used when navigation away from search occurs
-Category links now generate a viewNavigate event with search on and the
approriate category filter set. This is managed by a delegate in editorial
which fires the category on click, and a listener in the browser app which
creates and fires the viewNavigate event.

https://code.launchpad.net/~jcsackett/juju-gui/make-categories-work/+merge/167102

Requires: https://code.launchpad.net/~jcsackett/juju-gui/add-category-links/+merge/167131

(do not edit description out of merge proposal)

[j.c.sackett, 2013-06-03 17:24:39]

Please take a look.

[rharding, 2013-06-03 18:35:07]

LGTM but I'd like to chat and look at not creating a new event if we don't have
to as well as putting the category into the html as a data- attribute vs
parsing. See below.

https://codereview.appspot.com/9965044/diff/1/app/models/browser.js
File app/models/browser.js (right):

https://codereview.appspot.com/9965044/diff/1/app/models/browser.js#newcode95
app/models/browser.js:95: this.set('categories', []);
you can use setAttrs() to do this in one function call.

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

https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/browser.js#...
app/subapps/browser/browser.js:462:
this._editorial.on(this._editorial.EV_CATEGORY_LINK_CLICKED,
I'm not sure about putting this in the subapp here. Is there any reason the
editorial View cannot fire the normal event with search: true, filter: {...?

https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:44: EV_CATEGORY_LINK_CLICKED:
'category-link-clicked',
yea, this is a UI event triggering specific to the View. I really hate to have
this pulled by the subapp which shouldn't be worried about implementation
details like this.

https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:89: if (ev.target.get('tagName') !== 'A')
{
if you check currentTaget I believe it should always been the A node vs the
actual clicked element before bubbling. That would remove the need for this if
check.

https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
app/subapps/browser/views/editorial.js:94: var cat =
actual_target.get('id').replace('-link', '');
what do you think about modifying the html to supply a data-category-name and
just pulling that directly? I prefer the explicit "data is here" vs the tweaking
of strings found.

https://codereview.appspot.com/9965044/diff/1/app/templates/category-icons.pa...
File app/templates/category-icons.partial (right):

https://codereview.appspot.com/9965044/diff/1/app/templates/category-icons.pa...
app/templates/category-icons.partial:7: <a id="databases-link" href="">
this then doesn't need an id at all. Just data-category-id="databases"

[j.c.sackett, 2013-06-03 18:50:20]

On 2013/06/03 18:35:07, rharding wrote:
> LGTM but I'd like to chat and look at not creating a new event if we don't
have
> to as well as putting the category into the html as a data- attribute vs
> parsing. See below.
> 
> https://codereview.appspot.com/9965044/diff/1/app/models/browser.js
> File app/models/browser.js (right):
> 
> https://codereview.appspot.com/9965044/diff/1/app/models/browser.js#newcode95
> app/models/browser.js:95: this.set('categories', []);
> you can use setAttrs() to do this in one function call.
> 
> https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/browser.js
> File app/subapps/browser/browser.js (right):
> 
>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/browser.js#...
> app/subapps/browser/browser.js:462:
> this._editorial.on(this._editorial.EV_CATEGORY_LINK_CLICKED,
> I'm not sure about putting this in the subapp here. Is there any reason the
> editorial View cannot fire the normal event with search: true, filter: {...?

Only that you explicitly nixed that when we chatted, but I think maybe you were
misunderstanding and thinking there was a widget or something involved? I'm
happy to put it in the editorial view.

>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
> File app/subapps/browser/views/editorial.js (right):
> 
>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
> app/subapps/browser/views/editorial.js:44: EV_CATEGORY_LINK_CLICKED:
> 'category-link-clicked',
> yea, this is a UI event triggering specific to the View. I really hate to have
> this pulled by the subapp which shouldn't be worried about implementation
> details like this.

Yeah, see above.
 
>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
> app/subapps/browser/views/editorial.js:89: if (ev.target.get('tagName') !==
'A')
> {
> if you check currentTaget I believe it should always been the A node vs the
> actual clicked element before bubbling. That would remove the need for this if
> check.

Nope. The IMG and the SPAN both show up too. Thus the comment.
 
>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
> app/subapps/browser/views/editorial.js:94: var cat =
> actual_target.get('id').replace('-link', '');
> what do you think about modifying the html to supply a data-category-name and
> just pulling that directly? I prefer the explicit "data is here" vs the
tweaking
> of strings found.
> 
>
https://codereview.appspot.com/9965044/diff/1/app/templates/category-icons.pa...
> File app/templates/category-icons.partial (right):
> 
>
https://codereview.appspot.com/9965044/diff/1/app/templates/category-icons.pa...
> app/templates/category-icons.partial:7: <a id="databases-link" href="">
> this then doesn't need an id at all. Just data-category-id="databases"

Happy to give that a try.

[rharding, 2013-06-03 19:23:16]

On 2013/06/03 18:50:20, j.c.sackett wrote:
> On 2013/06/03 18:35:07, rharding wrote:
> > LGTM but I'd like to chat and look at not creating a new event if we don't
> have
> > to as well as putting the category into the html as a data- attribute vs
> > parsing. See below.
> > 
> > https://codereview.appspot.com/9965044/diff/1/app/models/browser.js
> > File app/models/browser.js (right):
> > 
> >
https://codereview.appspot.com/9965044/diff/1/app/models/browser.js#newcode95
> > app/models/browser.js:95: this.set('categories', []);
> > you can use setAttrs() to do this in one function call.
> > 
> > https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/browser.js
> > File app/subapps/browser/browser.js (right):
> > 
> >
>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/browser.js#...
> > app/subapps/browser/browser.js:462:
> > this._editorial.on(this._editorial.EV_CATEGORY_LINK_CLICKED,
> > I'm not sure about putting this in the subapp here. Is there any reason the
> > editorial View cannot fire the normal event with search: true, filter: {...?
> 
> Only that you explicitly nixed that when we chatted, but I think maybe you
were
> misunderstanding and thinking there was a widget or something involved? I'm
> happy to put it in the editorial view.

Ah, sorry if I mis-understood. It makes sense that I'd object to a widget doing
that event but Views are cool. 


https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
> > app/subapps/browser/views/editorial.js:89: if (ev.target.get('tagName') !==
> 'A')
> > {
> > if you check currentTaget I believe it should always been the A node vs the
> > actual clicked element before bubbling. That would remove the need for this
if
> > check.
> 
> Nope. The IMG and the SPAN both show up too. Thus the comment.

I can't replicate this. According to the docs and my experience ev.currentTarget
should always be the A tag regardless of what was clicked. Let's chat if you can
replicate this. It really shouldn't work that way.

http://yuilibrary.com/yui/docs/event/#basics

[j.c.sackett, 2013-06-03 20:33:05]

On 2013/06/03 19:23:16, rharding wrote:
> On 2013/06/03 18:50:20, j.c.sackett wrote:
> > On 2013/06/03 18:35:07, rharding wrote:
> > > LGTM but I'd like to chat and look at not creating a new event if we don't
> > have
> > > to as well as putting the category into the html as a data- attribute vs
> > > parsing. See below.
> > > 
> > > https://codereview.appspot.com/9965044/diff/1/app/models/browser.js
> > > File app/models/browser.js (right):
> > > 
> > >
> https://codereview.appspot.com/9965044/diff/1/app/models/browser.js#newcode95
> > > app/models/browser.js:95: this.set('categories', []);
> > > you can use setAttrs() to do this in one function call.
> > > 
> > >
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/browser.js
> > > File app/subapps/browser/browser.js (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/browser.js#...
> > > app/subapps/browser/browser.js:462:
> > > this._editorial.on(this._editorial.EV_CATEGORY_LINK_CLICKED,
> > > I'm not sure about putting this in the subapp here. Is there any reason
the
> > > editorial View cannot fire the normal event with search: true, filter:
{...?
> > 
> > Only that you explicitly nixed that when we chatted, but I think maybe you
> were
> > misunderstanding and thinking there was a widget or something involved? I'm
> > happy to put it in the editorial view.
> 
> Ah, sorry if I mis-understood. It makes sense that I'd object to a widget
doing
> that event but Views are cool. 
> 
> 
>
https://codereview.appspot.com/9965044/diff/1/app/subapps/browser/views/edito...
> > > app/subapps/browser/views/editorial.js:89: if (ev.target.get('tagName')
!==
> > 'A')
> > > {
> > > if you check currentTaget I believe it should always been the A node vs
the
> > > actual clicked element before bubbling. That would remove the need for
this
> if
> > > check.
> > 
> > Nope. The IMG and the SPAN both show up too. Thus the comment.
> 
> I can't replicate this. According to the docs and my experience
ev.currentTarget
> should always be the A tag regardless of what was clicked. Let's chat if you
can
> replicate this. It really shouldn't work that way.
> 
> http://yuilibrary.com/yui/docs/event/#basics

I went to all the trouble of doing a screencast to demonstrate I could reproduce
this, and then realized I was using `target`, not `currentTarget`. :-P

This is done too.

Changes coming in a moment.

[j.c.sackett, 2013-06-03 20:37:30]

Please take a look.

[jeff.pihach, 2013-06-03 21:20:39]

LGTM with trivial.

Thanks!

https://codereview.appspot.com/9965044/diff/10/app/models/browser.js
File app/models/browser.js (right):

https://codereview.appspot.com/9965044/diff/10/app/models/browser.js#newcode95
app/models/browser.js:95: this.set('categories', []);
This should be changed to this.setAttrs() as it will fire a change event for
each set() call vs one after they are all done.

https://codereview.appspot.com/9965044/diff/10/app/subapps/browser/views/edit...
File app/subapps/browser/views/editorial.js (right):

https://codereview.appspot.com/9965044/diff/10/app/subapps/browser/views/edit...
app/subapps/browser/views/editorial.js:91: }, 'A', this);
a

https://codereview.appspot.com/9965044/diff/10/test/test_browser_editorial.js
File test/test_browser_editorial.js (right):

https://codereview.appspot.com/9965044/diff/10/test/test_browser_editorial.js...
test/test_browser_editorial.js:164:
Y.one('#category-links').one('A').simulate('click');
a

[j.c.sackett, 2013-06-04 12:39:42]

On 2013/06/03 21:20:39, jeff.pihach wrote:
> LGTM with trivial.
> 
> Thanks!
> 
> https://codereview.appspot.com/9965044/diff/10/app/models/browser.js
> File app/models/browser.js (right):
> 
> https://codereview.appspot.com/9965044/diff/10/app/models/browser.js#newcode95
> app/models/browser.js:95: this.set('categories', []);
> This should be changed to this.setAttrs() as it will fire a change event for
> each set() call vs one after they are all done.

Ah, cool. Done.

> 
>
https://codereview.appspot.com/9965044/diff/10/app/subapps/browser/views/edit...
> File app/subapps/browser/views/editorial.js (right):
> 
>
https://codereview.appspot.com/9965044/diff/10/app/subapps/browser/views/edit...
> app/subapps/browser/views/editorial.js:91: }, 'A', this);
> a

Done.

> https://codereview.appspot.com/9965044/diff/10/test/test_browser_editorial.js
> File test/test_browser_editorial.js (right):
> 
>
https://codereview.appspot.com/9965044/diff/10/test/test_browser_editorial.js...
> test/test_browser_editorial.js:164:
> Y.one('#category-links').one('A').simulate('click');
> a

Done.

[j.c.sackett, 2013-06-04 16:45:37]

*** Submitted:

Adds handling to category links

-Filter model now has a reset mechanism
-Filter model's reset is used when navigation away from search occurs
-Category links now generate a viewNavigate event with search on and the
approriate category filter set. This is managed by a delegate in editorial
which fires the category on click, and a listener in the browser app which
creates and fires the viewNavigate event.

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