Code review comment for lp:~danilo/launchpad/bug-455771

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Kudos for fixing this. Two points though:

1. You now have a conditional top-portlet div. Can that be fixed by creating a separate top-portlet and then putting the conditional div either below or inside it?

2. It's not clear what the additional test is testing. Always a problem with "look, it doesn't blow up" fixes. I'd suggest giving the div an HTML id and having the test show its absence.

Hope we can get through this soon, since I'm EOD and I'm sure you want this branch done.

review: Needs Fixing

« Back to merge proposal