Since the new functionality for theme('table' allows us to place headings in the body of a table (http://drupal.org/node/76741), this patch is to clean up the presentation of tables;
without top headers by adding a thin grey top border to tbody,
separation between thead headings and tbody headings and
headings in tbody only has a thin bottom border.

Removed redundant overwrite in watchdog module for headings in the body

To see an implementation: http://drupal.org/node/78105

CommentFileSizeAuthor
#12 before_after_tables_styles.png20.9 KBnickl
#11 tables-styles_3.patch536 bytesnickl
#6 after_without_heading.png9.78 KBnickl
#5 before_without_heading.png9.88 KBnickl
#4 after_with_heading.png9.94 KBnickl
#3 before_with_heading.png10.04 KBnickl
tables-styles.patch1.12 KBnickl

Comments

nickl’s picture

nickl commented
Assigned: Unassigned » nickl
Status: Active » Needs review

Please review...

dries’s picture

dries commented

Attach a before and after screenshot?

nickl’s picture

nickl commented
StatusFileSize
new before_with_heading.png10.04 KB

Used the watchdog event table as an example, there are two conditions that this patch fixes;
table with thead headings and tbody headings and
table without thead headings and tbody headings

nickl’s picture

nickl commented
StatusFileSize
new after_with_heading.png9.94 KB

After with heading

nickl’s picture

nickl commented
StatusFileSize
new before_without_heading.png9.88 KB

Before without heading

nickl’s picture

nickl commented
StatusFileSize
new after_without_heading.png9.78 KB

After without heading

m3avrck’s picture

m3avrck commented
Status: Needs review » Needs work

Looks good, however I would use defaults.css instead of system.css

Defaults.css is for more generalized "default" styles for regular elements. Would be best fit there. After that change this sounds good to me!

nickl’s picture

nickl commented
Status: Needs work » Needs review

I also thought default.css but the rest of the table styles, i.e. the odd and even rows, I found in system.css and thus added this patch there:

/*
** HTML elements
*/
th.active img {
  display: inline;
}
tr.even, tr.odd {
  background-color: #eee;
  border-bottom: 1px solid #ccc;
  padding: 0.1em 0.6em;
}
td.active {
  background-color: #ddd;
}

Can we have more comments on this please.

m3avrck’s picture

m3avrck commented

Right, those are a bit off, debatable. defaults.css should really be about generalized defaults, mostly to do with paddings, margins, offsets, really shouldn't be *theme* related.

However, that might out of the scope of this patch--sorry for the feature creep. :-)

For now, they make sense in either place, I'll work on another issue to further clean and seperate that later, that shouldn't hold this up.

dries’s picture

dries commented

The visual improvements look good, but I leave it up to m3avrck to approve the underlying CSS changes.

nickl’s picture

nickl commented
StatusFileSize
new tables-styles_3.patch536 bytes

@Dries Thank you for the comments.

@m3avrck Lets KISS, Rome wasn't built in one day. Here's the bare minimum that should go in. This now only patches watchdog.css to only add theme related fixes, for watchdog event table to clean the table without headings. We can do clean ups after code freeze.

nickl’s picture

nickl commented
StatusFileSize
new before_after_tables_styles.png20.9 KB

The new before after...

m3avrck’s picture

m3avrck commented

Nickl, I'm a bit confused why you posted the bare-minimum patch, the first one you posted was certainly ok. I was more concerned about organization than anything :-)

Anyways, Dries, the first patch Nickl posted is good to go for CSS. After looking through the CSS in Drupal again, his patch is consistent with the current behavior so should be committed.

We'll have another issue later to clean things up a bit more.

nickl’s picture

nickl commented
Status: Needs review » Reviewed & tested by the community

Since we do all agree this is the patch: http://drupal.org/files/issues/tables-styles.patch RTBC

m3avrck’s picture

m3avrck commented

Yes, that looks good to me, ready to commit.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Looks like this does need more cleanup.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)