Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#6 | watchdog_event_table_2_0.patch | 2.59 KB | nickl |
#4 | watchdog_event_table_1_0.patch | 2.59 KB | nickl |
watchdog_event_table_0.patch | 2.77 KB | nickl | |
Comments
Comment #1
drumm- $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.
Comment #2
drummComment #3
nickl CreditAttribution: 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:
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
Comment #4
nickl CreditAttribution: nickl commentedAfter 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:
Still left the $attribute variable for readability.
@drumm thank you for your patience and guidance.
Comment #5
nickl CreditAttribution: nickl commentedSeperate table styles patch: http://drupal.org/node/79346
Comment #6
nickl CreditAttribution: nickl commentedNew patch roll to head... style sheet changes.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedtested this on a fresh HEAD, and it works as advertised. RTBC?
Comment #8
nickl CreditAttribution: nickl commentedMarked as RTBC since no further comments have been made since justinrandell reviewed this.
Lets move this forward...
Comment #9
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks nicki.
Comment #10
nickl CreditAttribution: nickl commented@Dries Glad I can help... lets roll the next one!
Comment #11
(not verified) CreditAttribution: commented