Code review comment for lp:~spiv/launchpad/bmp-inline-diffs

Revision history for this message
Данило Шеган (danilo) wrote :

Andrew, just a few minor comments:

 - getDiffUrl could probably be dropped and replaced with bmpGetDiffUrl use (i.e. bmpGetDiffUrl(revno, revno-1))
 - perhaps you could accept undefined (and use Y.Lang.isUndefined or Y.Lang.isValue [which is false for nulls and undefineds]) as the end_revno in bmpGetDiffUrl when you just want one revision
 - with the changes to the expander in the meantime, it'd probably be better to instanciate the expander directly instead of using createByCSS. That will allow you to call expander.setUp(true), when the content inside the "toggle" node will be turned into a proper link (and you shouldn't wrap it inside <a> in TAL, or it'll end up being <a> inside an <a>)
 - alternatively to the above, you can just add 'href="#"' to get the pointing hand cursor to appear if you feel like the link should stay in TAL or you prefer createByCSS API
 - generally, tables should not be used for non-tabular-data formatting, but if existing CSS classes you use don't work if you simply replace <td>s with <div>s, just keep it as is
 - you may also need to change test set-up (especially in HTML) because of the recent changes to the test infrastructure; try looking at other existing tests for reference and make sure the test passes when run as "bin/test -t test_branchrevisionexpander.html"

I consider most of the above minor issues, so here's a conditional approval on fixing those listed above.

review: Approve

« Back to merge proposal