launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Review: Needs Fixing code


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

- 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. 

- 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.

  
- 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.

- 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.



[1]: http://yuilibrary.com/yui/docs/attribute/#attrsetflow
[2]: http://yuilibrary.com/yui/docs/widget/
[3]: http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/view/head:/lib/lp/app/javascript/indicator/indicator.js#L64
-- 
https://code.launchpad.net/~jcsackett/launchpad/banner-class/+merge/104984
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.