launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

> - The init base_cfg stuff is unnecessary. The values passed into as cfg will
> automatically match up and set ATTR properties for you. [1]

Cool, I'll look into that. It felt sort of redundant hackish, but I wasn't clear on a better way.

> - If you're going to have renderUI you should just extend Y.Widget in your
> Base.create() as it'll give you the full renderUI/bindUI and you get free
> show/hide methods. [2] So you're show/hide turn methods triggered by the
> render event or watching the visible attritute with this.on('visibleChange')
> [3]
> 
>   This also effects your css since the widgets automatically get -visible css
> properties and you'd not have to add that yourself.

Hm. I think I need to talk more with you on that--I thought that by passing in Y.Widget as the second arg I was already extending from that, and that to get my own things happening in bind/render show/hide I had to write my own anyway, just got cycle taken care of. I guess I'm not sure what this actually means in concrete terms?

I'm also not sure about the relevance of the -visible css, given the banner notifications already have their own css stuff defined elsewhere.

> - I'm not sure the reason for the hard coded body element. I *think* if I
> understand correctly it's only because it's the parent of the notification. In
> a change to the widget based approach you get an attribute 'boundingBox' that
> helps you find the immediate parent of your widget. In this way the code would
> be more reusable (say I wanted to stick one of these on a div somewhere in a
> document vs the whole page) but also if the html were to be moved in any way
> that the body is no longer the direct parent node.

If you're referring to the stuff running off Y.one('body), it's because the notifications *require* modification of body css, and changing that is out of scope for this arc of work. I'm not sure about beta-notifications, but privacy/global-notification stuff is all managed on the body element in our CSS, and since beta-notificaion is based off those, it probably stands. Since this module is to be the parent for those two, it needs to incorporate that. I'm open to filing a but about that being a problem, however.

> - In the tests, I'd just remove the maincontent div from the .html and move
> the code to generate it to the setUp method. It's a bit strange to see node
> creation in teardown. You also end up creating another login/logout div it
> appear.

Yeah, I was torn on how best to deal with this. I'll clean it up per your suggestions.

> - A final test tip, I've before used an extra flag 'skip_animation' so that in
> tests I could not worry about timing issues and it generally helps test
> fragility in full test runs.

I like that--I guess that would just set the anim time to 0?
-- 
https://code.launchpad.net/~jcsackett/launchpad/banner-class/+merge/104984
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.