| Message ID | 1440440620-25937-26-git-send-email-damien.lespiau@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
> From: Belén Barros Peña <belen.barros.pena@intel.com> > > A bit more spacing everywhere. Also opted for the highlight on hover > instead of the alternating row background color. > > v2: Squash the patch fixing the unit tests (Jeremy Kerr) > > Signed-off-by: Belén Barros Pena <belen.barros.pena@intel.com> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> Looks good but I can imagine this being contentious. This change means we can display a lot less information on the screen just so things look a little prettier. I'm hesitant about this, but I'll allow it. If I may, though, I'd like to make suggest one change that would make a huge difference though: monospace font for the subject. A monospace font makes it very easy to visually distinguish groups of related changes, given the way that these subjects are formatted (with the leading tags, etc.). Just a thought. Acked-by: Stephen Finucane <stephen.finucane@intel.com>
> On 09/09/2015 16:13, "Finucane, Stephen" <stephen.finucane@intel.com> > wrote: > > >> From: Belén Barros Peña <belen.barros.pena@intel.com> > >> > >> A bit more spacing everywhere. Also opted for the highlight on hover > >> instead of the alternating row background color. > >> > >> v2: Squash the patch fixing the unit tests (Jeremy Kerr) > >> > >> Signed-off-by: Belén Barros Pena <belen.barros.pena@intel.com> > >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > > >Looks good but I can imagine this being contentious. This change means we > >can display a lot less information on the screen just so things look a > >little prettier. > > Well, not exactly: the looking 'prettier' is somehow an after effect. > Apparently white space improves readability (speed and comprehension), > both real and perceived [1]. That might be one of the reasons why we find > them prettier :) > > [1] > http://usabilitynews.org/reading-online-text-a-comparison-of-four-white-spa > ce-layouts/ That particular article relates to blocks of text rather than tabular data so I'm not sure it's relevant. I'm happy with the additional whitespace requirements though: just pointing out that it might be contentious :) > >I'm hesitant about this, but I'll allow it. > > > >If I may, though, I'd like to make suggest one change that would make a > >huge difference though: monospace font for the subject. A monospace font > >makes it very easy to visually distinguish groups of related changes, > >given the way that these subjects are formatted (with the leading tags, > >etc.). Just a thought. > > This is interesting: I wonder if other folks find the same thing. I would > have no problem with the font change. I wonder how'd you'd determine this? Some form of A/B testing (or other usability testing method)? Try it yourself though: I think it's a marked improvement. > Cheers > > Belén > > > > >Acked-by: Stephen Finucane <stephen.finucane@intel.com>
diff --git a/htdocs/css/style.css b/htdocs/css/style.css index e09821e..1ec7400 100644 --- a/htdocs/css/style.css +++ b/htdocs/css/style.css @@ -106,34 +106,6 @@ dl dt { color: green; } -/* patch lists */ -table.patchlist { - width: 98%; - border: thin solid black; - padding: 0em 1em; -} - -table.patchlist th { - background: #eeeeee; - border-bottom: thin solid black; - text-align: left; - padding-left: 6px; -} - -table.patchlist th img { - vertical-align: bottom; -} - -table.patchlist td { - padding: 2px 6px 2px 6px; - margin: 0px; - margin-top: 10px; -} - -table.patchlist td img { - vertical-align: bottom; -} - .filters { border: 1px solid #cccccc; border-radius: 4px; @@ -149,21 +121,6 @@ a.filter-action:hover { } -table.patchlist td.patchlistreorder { - background: #c0c0ff; - border-top: thin solid gray; - border-bottom: thin solid black; - font-size: smaller; - text-align: right; -} -table.patchlist tr.odd { - background: #ffffff; -} - -table.patchlist tr.even { - background: #e8e8e8; -} - a.colinactive, a.colactive { color: black; text-decoration: none; diff --git a/patchwork/templates/patchwork/patch-list.html b/patchwork/templates/patchwork/patch-list.html index 6d57a66..628696e 100644 --- a/patchwork/templates/patchwork/patch-list.html +++ b/patchwork/templates/patchwork/patch-list.html @@ -39,7 +39,7 @@ {% csrf_token %} <input type="hidden" name="form" value="patchlistform"/> <input type="hidden" name="project" value="{{project.id}}"/> -<table class="patchlist" id="patchlist"> +<table class="table table-hover" id="patchlist"> <thead> <tr> {% if user.is_authenticated %} @@ -140,7 +140,7 @@ {% if page.paginator.count %} <tbody> {% for patch in page.object_list %} - <tr id="patch_row:{{patch.id}}" class="{% cycle 'odd' 'even' %}"> + <tr id="patch_row:{{patch.id}}"> {% if user.is_authenticated %} <td> <input type="checkbox" name="patch_id:{{patch.id}}"/> diff --git a/patchwork/tests/test_list.py b/patchwork/tests/test_list.py index c7fbbea..f440e3e 100644 --- a/patchwork/tests/test_list.py +++ b/patchwork/tests/test_list.py @@ -70,7 +70,7 @@ class PatchOrderTest(TestCase): patch.save() def _extract_patch_ids(self, response): - id_re = re.compile('<tr id="patch_row:(\d+)" ') + id_re = re.compile('<tr id="patch_row:(\d+)"') ids = [ int(m.group(1)) for m in id_re.finditer(response.content) ] return ids