Make Report table better

How about this? http://img.skitch.com/20100501-1d5tkhubw5ajmnj76c3771d216.jpg

Designwise I give this a thumbs up. Its a huge improvement to what it looked like before.

Codewise, "table.system-status-report tr.info > *" is kind of weird. At the same time, the fact that it has to select th and td and apply background colors to both is pretty weird, so its acceptable given the improvement.

FYI "> *" means "any element that is a direct child. Thus tr.info > * only selects direct children, not elements within... (learn something new every day) its css 2.1 so i'm marking this rtbc.

see:http://www.456bereastreet.com/archive/200510/css_21_selectors_part_2/

Agreed that it looks much better. Committed to CVS HEAD.

I agree that the CSS looks kind of weird, so I'm marking this 'needs work' to evaluate if we can make it better.

Improvements will have to happen at the markup level. The root of the issue is
1. the design relies on alternating colors to separate rows with classes(e.g .odd,.even)
2. rows also have classes which change the color (e.g. .info, .warning, .dennis)
3. the classes are shared at the row level e.g. <tr class="odd dennis warning"> </tr>
4. the left column uses a [th] tag which while perfectly valid is bound to clash with the assumptions of most designers writing table styles.
Given the non ideal markup, the CSS is actually quite elegant as it is...

Heh... downright curious as to what "ie 6 support" means in seven.

Consider this: this patch didn't introduce "tr.info > *". I did however introduce tr.info.odd which IE6 partially supports "tr.info.odd" IE 6 reads as "tr.odd".

To be perfectly honest, I'm not TOO worried about IE6 support in this specific use case. Its not like this is a form that's visible to 99% of drupal site visitors. It really should be visible to only under 1 percent...

After all - please read the actual text that's in your screenshot: http://drupal.org/files/issues/Status-report_0.jpg . (remember, this page tells you things like "a module has a known exploit open, and needs to be updated"). I'll be blunt, if someone needs to use this page AND believes drupal needs to support IE6 everywhere god help them. THey aren't ready to manage drupal installs yet... :-/

Case and point. Open up "www.google.com" in IE6 and look in the top right corner.

***
In anycase, my humble opinion is that doubling up the rules like that makes the CSS more complicated, not less.... I think we should fix the markup, then writing simple CSS will come naturally...

I don't really agree with you Nick. The markup is as simple and semantic as it could be. What changes do you propose making?

(This patch maintains the zebra striping, but doubles up. and also follows code standards)

+table.system-status-report tr.even.error th,
+table.system-status-report tr.even.error td {
   background-color: #ecc;
 }

Okay. This will work in IE6 in theory. Not perfectly... but tr.even.error will depreciate to tr.error in IE6 world (in theory).

We're doubling the size of the rule... I hope we really love IE 6, because remember - the last version was perfectly valid CSS to modern browsers and was less than half the size...

RE markup:

I get this is technically valid.

<table>
<tr><th></th><td></td></tr>
<tr><th></th><td></td></tr>
</table>

The real jokes is you can't distinguish the design between tr th, tr td -- but we have to double up your CSS rules to account for them... Why are we doing this? Anyone?

Does it benefit screen readers?
What makes the left column a table header in this case anyways? The lack of thead at the top?

If its a semantic thing, than we should consider that maybe tables are the wrong format for this page's job? This in short is what's bugging me about the markup.
IN any case had the

I'm not doing that because I love IE 6. I'm doing it because > * is somewhat unclear (yes, it does have a specific meaning. but it's bad for DX)

th = table heading, td = table data. "Node access permissions" is clearly a heading. "Disabled" is data.

"Node access permissions" is clearly a heading.
If that's clearly a heading, then /admin/people/permissions table rows should read:

[th]Administer blocks(table heading)[/th]
--[td]Role 1 checkbox(table data)[/td]
--[td]Role 1 checkbox[/td]

[th]Administer users[/th]
--[td]Role 2 checkbox[/td]
--[td]Role 2 checkbox[/td]

That would be both silly, and interfere with the horizontal table headings:
[thead]
--[th]Permission[/th]
--[th]role1[/th]
--[th]role2[/th]
[/thead]

Likewise I would argue that the real table headings on the reports page are missing but implied... e.g.:
[thead]
--[th]component[/th]
--[th]status[/th]
[/thead]
[tbody]
--[td]Node access permissions[/td]
--[td]disabled[/td]
[/tbody]

Not sure I see what's incorrect about doing it this way. Especially given the amount it will simplify the CSS, and the fact that I can't find a single other drupal page that uses [th] in [tbody]

In any case, this patch removes a lot of CSS from seven. The changes to the system page are straightforward enough... The CSS is stupid simple. Again, no idea what we gain besides more complexity in our CSS from using [th] in [tbody].

Committed to CVS HEAD. Thanks.

What about the fact that this removes the zebra stripes?

When I do a design review, I look primarily at the function of the design. In the case of zebra stripes you first posted, I marked it as ready because it separated rows.

Once you add borders to the rows as suggested by droplet (and included in your last patch) the stripes could be considered a flourish or noise depending on personal taste.

Personally, I prefer
http://drupal.org/files/issues/Status-report_0.jpg
to
http://img.skitch.com/20100501-1d5tkhubw5ajmnj76c3771d216.jpg

Extra colors are just extra noise on that page. However, my design preference isn't the primary reason for removing them. Rather its that it achieves the exact same end (separating rows), but
1. with 1/2 the CSS (seven does not need more CSS)
2. Uses techniques that will render consistently in IE 6 through chrome.

I consider this bug fixed, but am open to an argument for why zebra stripes are needed.

Going to just close this for now. I've got a lot of ideas about that table, but most of them involve enough code changes that they are likely to make webchick want to set buildings on fire.

Not sure you are reading it right.
-table.system-status-report tr.error.even > * {
- background-color: #ecc;
-}
tr.even,
tr.odd {
border-width: 0 1px 0 1px;

This indicates no change to borders. However the change you are suggesting touches every table not just this one....

Nick, i didn't notice the borders - thought there was no distinction between table cells :-).

You win.