Proposed commit message:
Issue #1842326 by c4rl, sun, joelpittet, pakmanlh, jjcarrion | jenlampton: Merge _theme_table_cell() into theme_table().
One of the principles we arrived at during the last Drupal Twig sprint was that HTML elements like UL and TABLE should not be divided into smaller parts. A table is only whole if it contains cells, and a table cell can not stand on it's own. We want Drupal's theme system to match the mental model of front-end developers, and semantically match HTML.
_theme_table_cell() is just a shortcut to write a few lines of HTML (and consequently an example of when DRY becomes an anti-pattern).
Proposed solution: theme_table() refactors calls to _theme_table_cell() to simply mirror its functionality in theme_table(). Forum module also has a call to be refactored in a similar fashion. (Sidenote: Forum module theme functions need some heavy revision, let's save that discussion for other issues).
Comment | File | Size | Author |
---|---|---|---|
#10 | interdiff.txt | 11.06 KB | sun |
#10 | theme.tablecell.10.patch | 12.02 KB | sun |
#6 | drupal-1842326-6.patch | 7.4 KB | c4rl |
#6 | drupal-1842326-6.patch-interdiff.txt | 709 bytes | c4rl |
#4 | drupal-1842326-4.patch | 7.42 KB | c4rl |
Comments
Comment #0.0
jenlamptoncleanup
Comment #1
jenlamptonOn closer consideration, this is a good candidate for a renderable object :)
Comment #2
c4rl CreditAttribution: c4rl commentedTable cells are always part of a larger structure (a table) and do not need to be themed independently. theme_table_cell just isn't necessary.
Related: #1842160: Consider appropriate usage of theme() from within Twig templates
Comment #3
steveoliver CreditAttribution: steveoliver commentedWe realized this was necessary while working on #1842160: Consider appropriate usage of theme() from within Twig templates and finishing #1778968: Convert theme_table to Twig.
Comment #4
c4rl CreditAttribution: c4rl commentedOkay the original title makes the most sense. On further inspection, this is just a helper function. The attached patch removes this function and should make it easier to proceed with the Twig theme_table conversion.
Comment #6
c4rl CreditAttribution: c4rl commentedI see the issue, there's an empty class attribute on td tags.
Comment #6.0
c4rl CreditAttribution: c4rl commentedclarify? iz tired
Comment #7
alexrayu CreditAttribution: alexrayu commented#6 fixed the empty td classes for me. The patch applied well and the form rendered correct.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedThe patch no longer applies, unfortunately.
error: patch failed: core/modules/forum/forum.module:6
error: core/modules/forum/forum.module: patch does not apply
Comment #8.0
thedavidmeister CreditAttribution: thedavidmeister commentedExpanded summary.
Comment #9
joelpittetDarn I didn't see this issue. I've duplicated it over at #2216407: Remove _theme_table_cell().. The forum bits were removed in #2092343: Consolidate forum.module and remove call to _theme_table_cell() within template_preprocess_forum_topic_list().
Could someone double check the pieces in this and see if anything is needed further at #2216407: Remove _theme_table_cell().?
Comment #10
sunI worked on this, and cherry-picked code from both the patch here as well as @joelpittet's patch in #2216407: Remove _theme_table_cell(). (duplicate now)
Comment #11
joelpittet@sun Thank you for refactoring and still keeping some of the variables and logic we put work into the other patch!
I also like how you took the $cell['attributes'] and $cell['data'] off the $cell into their local variables because there is less juggling that way:)
Hopefully this goes green!
Comment #12
joelpittetThis removes both _theme_table_cell() and tablesort_cell() functions which puts the markup logic and addition of extra classes in context of where they are being built instead of passing the context into helper functions that seemed to become more complex th and td needed different conditions for handling their display.
This adds new tests and makes them faster (UnitTest now vs SimpleTest before).
Did a quick manual test using simply test me on the /admin/reports/dblog page which has the sort indicators and a fairly large table and the markup is identical.
It passes previous tests.
I also reviewed the code as well to see if there were any glaring mistakes or oversights that I could see and none came to mind.
RTBC++
Comment #13
star-szrAdding proposed commit message to pull in the folks who worked on #2216407: Remove _theme_table_cell()..
Comment #14
catchCommitted/pushed to 8.x, thanks!
Comment #16
sun