Since the new table header feature has been committed to cvs (http://drupal.org/node/76741), this patch is to demonstrate the new functionality of defining a table cell as a heading, by removing the custom theme theme_watchdog_event and replacing it with the standard theme_table on the watchdog event details node (admin/log/event/1234) as per Morbis Iff's suggestion.

CommentFileSizeAuthor
#6 watchdog_event_table_2_0.patch2.59 KBnickl
#4 watchdog_event_table_1_0.patch2.59 KBnickl
watchdog_event_table_0.patch2.77 KBnickl

Comments

drumm’s picture

drumm
he/him
Location NY, US
commented

- $header should be an empty array.
- Brief arguments like $header and $attributes do not need to be separately defined as variables, just put the value where it belongs.
- We put a comma after /every/ array element. PHP doesn't care about the extra elements, and adding new elements becomes as easy as adding a new line.
- Cells with only 'data' defined, don't need the extra array/data construct.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Needs work
nickl’s picture

nickl commented

@drumm
Thank you for the review... much appreciated!
Thesis: To demonstrate the new capabilities of theme('table'
Making $header an empty array does not have the same effect as:

$header = array(
      array('colspan' => 2)
    );

Although the table does not have a heading it looks much neater having an empty <hr></hr>

The $attributes $rows $header variables are not empty and more easily readable by example and understandable as presently coded.
I changed the 'data' => identifier and IMHO it makes the code less readable. Any comments?
I don't understand what you mean by "comma after /every/ array element" please explain.

Philosophy: Any monkey can be taught to write code that computers understand developers write code people can understand. YMMV

nickl’s picture

nickl commented
Status: Needs work » Needs review
StatusFileSize
new watchdog_event_table_1_0.patch2.59 KB

After IRC discussions the following conclusions have been reached and patched accordingly.
Empty tags are bad mark up - resolution: replaced $header with array() need to patch css for presentation.
Removing 'data' => for empty cells is cleaner - resolution: removed the redundant array
Comma after last entry in multi-lined array is part of the drupal coding standards - resolution:

   $rows = array(
      array(
        array('data' => t('Type'), 'header' => TRUE),
        t($watchdog->type),
      ),
   );

Still left the $attribute variable for readability.

@drumm thank you for your patience and guidance.

nickl’s picture

nickl commented

Seperate table styles patch: http://drupal.org/node/79346

nickl’s picture

nickl commented
StatusFileSize
new watchdog_event_table_2_0.patch2.59 KB

New patch roll to head... style sheet changes.

Anonymous’s picture

Anonymous (not verified) commented

tested this on a fresh HEAD, and it works as advertised. RTBC?

nickl’s picture

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

Marked as RTBC since no further comments have been made since justinrandell reviewed this.
Lets move this forward...

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks nicki.

nickl’s picture

nickl commented

@Dries Glad I can help... lets roll the next one!

Anonymous’s picture

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