Code review comment for lp:~wallyworld/lazr-js/activator-flash-node

Revision history for this message
Deryck Hodge (deryck) wrote :

This looks good, thanks!

I'll just not for future reference that the activator widget breaks from good YUI widget design, by declaring all attributes local on "this". The ATTRS object is unused -- i.e. this.some_node rather than this.get('some_node'). It's fine how you've done your code, following the pattern already there, but good widget design would store this node in ATTRS and take advantage of YUI's widget framework. Most of the other widgets do this correctly in lazr-js.

As I say, the way you've done it here is fine. Just wanted you to know, if you didn't already. I wouldn't ask you to refactor the widget to follow the others, just for this simple change.

Cheers,
deryck

review: Approve (code)

« Back to merge proposal