Code review comment for lp:~dholbach/harvest/504299

Revision history for this message
Dylan McCall (dylanmccall) wrote :

Yay! It looks good, but there are a few things.

For code readability, it would be good to move your changes in filters/filters.py over to opportunities/filters.py instead.
filters/filters.py is generic so far, and it would be nice to keep it that way :)
So, we'd be overriding render_html_value_choice in OppListFilter.

I think I would prefer a really simple symbolic style for that help icon. One that matches the text colour, so it doesn't shout for attention. The rest of the site's design gives any bright colour a very powerful presence. That can wait a little while, though (unless you have something handy!).
The help icon is 14px, but the text size is 12px. I don't think it's causing any trouble for spacing (because it's a floated element and there's 5px extra padding on the bottom of the list item), but if someone is already thinking of a nice monochrome icon, one that looks good at 12px would be cool.
Being a fan of plain text, I tried scratching the image and using just a bold question mark character. I thought it looked rather nice — especially with the very pretty UbuntuBeta font, which I'm sure it can use happily some day. It also gives us a lot of room to play with colours. I tried giving it the colour we use for other links — rgb(110,64,84) — and it seemed okay :)
http://people.ubuntu.com/~dylanmccall/harvest/media/gsoc-week11/opportunitylist-questionmark.png

Speaking of floated elements, we need this…
Below line 204:
#filters > .filtergroup > .filter-value > ul > li
Add:
clear:both;
That will force the list item to always add a line between itself and a floated element. (Very important if the help icon wraps to a new line).

The link for the explanation has the explanation itself as the url right now. Probably the best thing to do is a new view function to get an opportunity set's explanation. (Hm… and other details? Could be fun!). It could just be ridiculously simple for now and we'll make it all fancy later on.

I haven't done it yet, but I think you'll want to use:
from django.core.urlresolvers import reverse
(The reverse function works just like the {% url url_name %} template tag).

Thanks!

review: Needs Fixing

« Back to merge proposal