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.
Following on from http://drupal.org/node/98696, and as discussed with Bèr Kessels on IRC;-
This splits up the logic and themeing for theme_item_list() making the theme function much simpler, using a helper function for the complex bit, and creating a new theme function to theme the item itself.
Also fixes a couple of possible bugs. Initialising $data and extra \n before first <li> (maybe not needed, but ‘as advertised’).
Comment | File | Size | Author |
---|---|---|---|
#43 | drupal8.theme-item-list.43.patch | 9.11 KB | sun |
#41 | drupal8.theme-item-list.40.patch | 9.11 KB | sun |
#37 | drupal8.theme-item-list.37.patch | 9.08 KB | sun |
#35 | drupal8.theme-item-list.35.patch | 9.09 KB | sun |
#26 | edge-HEAD.theme-item-list.26.patch | 7.76 KB | sun |
Comments
Comment #1
zeta ζ CreditAttribution: zeta ζ commentedSorry, no-one is going to review this unless I mark it CNR :-)
Comment #2
floretan CreditAttribution: floretan commentedGood idea. A couple suggestions though:
Comment #3
zeta ζ CreditAttribution: zeta ζ commentedThanks for reviewing this.
$output .= '...';
, but twisted? One line is out of order, but are you referring to this, or too many.
’s, or embedding$output
in the RHS?I think it is good practice to add opening and closing tags on the same line, building outwards in a co-ordinated way. Would double quoted stings make it more readable?;-
or a template file?
Comment #4
floretan CreditAttribution: floretan commentedRegarding point 1, I see what you're doing now: building the HTML from the inside rather than from the beginning. Even though it's not really standard, it does makes sense. Maybe a little comment would make it clearer.
For point 3, theme functions from .inc files (like theme_item_list()) are all defined in drupal_common_theme() inside of common.inc.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedAbout #3.1: in that case single-quoting is better than double quoting in that case.
About the string order, it is more conventional in Drupal (and arguably easier to read), to build the output in the text flow order. Something like that will do:
By the way: there was a bug in your patch: the
<h3>
should go inside the<div>
but outside the<ul>/<ol>
.Comment #6
zeta ζ CreditAttribution: zeta ζ commented<div>
the</div>
is right there, just begging to be kept in sync. Each line stands up for itself (once I get it right :-( ). Thanks for the catch Damien, I had put this in #3-1. but didn’t make a new patch.Comment #7
cburschkaJust as a question, is there some legacy reason why we call it an "item_list" rather than just a "list"? I can't think of any definition of a "list" that does not consist of "items". After all, we don't call it "theme_cell_table" either.
This is particularly weird with the name "theme_item_list_item".
Perhaps an opportunity to clean up the API a bit more?
Also, patch no longer applies.
Comment #8
JohnAlbinAgree that theme_list seems better then theme_item_list.
But pushing to D8.
Comment #9
sunSimilar to #98696-15: Various bugs in theme_links(), I think we can resolve this issue easily.
Although it's true that theme_item_list() is relatively complex, the reason for that is its current programming logic only; mainly the inner foreach to handle nested child lists.
Simplifying that inner code automatically resolves the overall issue. (I recommend to hit the "Hide deletions" button of Dreditor)
It could only further simplified by adding a preprocess function for theme_item_list(), but I don't really like that idea.
Comment #10
sunComment #12
sunSorry.
Comment #13
sunAny feedback? I really think that this is a major improvement compared to the current theme function body.
Comment #14
Jacinesubscribing.
Comment #15
JacineOverall, I find the patch in #12 makes this a lot easier to read and understand. It's a really nice cleanup and I don't see any reason why it should have to wait until D8 (though
theme_list()
would be nice in D8).The current version in core is pretty confusing. For example, this used to trip me up a lot when trying to adding zebra striping classes using the same, which is usually the sole reason I need to go in and override it:
This still confuses me:
This patch makes it MUCH better. I've tested this and think it's ready.
Comment #16
sun@Dries/webchick: I'd like to repeat that it's easier to see the resulting massive simplification of this patch by hitting the "Hide deletions" button of Dreditor. Without doing so, the changes look quite scary, but they are definitely not - it's only the resulting logic that matters.
Comment #17
sun#12: drupal.theme-item-list.12.patch queued for re-testing.
Comment #18
sunComment #19
Dave ReidJust because someone is upset doesn't mean we have to mark something as won't fix. Someone else is more than welcome to pick up work on a ticket.
Comment #20
webchickLooks like a nice TX improvement, but too late for Drupal 7.
Comment #21
sunComment #22
chx CreditAttribution: chx commentedComment #23
sunThis issue will be moved back into the D8 queue, after it has been committed to Edge 7.x.
Comment #24
sunMassively improved the theme_item_list() tests, which revealed quite some bugs in the nested child list handling, as well as empty list handling.
Just ran the tests with core's theme_item_list(), and yeah, it outputs a list container and heading even if there is no list, and the attributes handling for nested child list is utterly broken.
Comment #25
JacineHmm, when I test this with the pagers on the tracker page, I get empty h3's, and in the recent comment block, I don't. I looked at the variables and the pager title is NULL, while the recent comment block title is an empty string. :/
Comment #26
sunFurther improved the tests to account for the item list title. We just found out and discussed that theme_pager() is guilty for those stale headings and that needs to be fixed in core.
Comment #27
JacineThanks :)
Tests pass and it looks good.
Comment #28
sunMoving back to core.
Comment #29
chx CreditAttribution: chx commentedThis was RTBC before . Why is it CNW now?
Comment #30
sunReasonable question. That is, because theme_item_list() in core
And lastly, the last patch for core did not sufficiently account for most of these bullets, so it has to be redone based on the committed patch.
Happy to clarify more, in case I missed something.
Comment #31
zeta ζ CreditAttribution: zeta ζ commented#30 5. Does this help? Add zebra striping to theme_item_list()
Comment #32
sunI'd be most grateful if someone would be able replace the code in core 1:1 with the code + tests in http://drupal.org/project/edge.
It's all ready and merely a matter of replacing it. Even tested on D7 production sites.
Comment #33
tim.plunkettI'll take a crack at this, probably tomorrow.
Comment #34
tim.plunkettOh wow, I sure let this slide. Haven't had the time.
Comment #35
sunFrom Edge with love.
Comment #36
JacineThis is just a tiny nitpick but I find "...:" in this comment odd. Can we change or drop it or is it just me? Other than that, this is ready and has been since it got committed to Edge months ago thanks to you. ;)
Comment #37
sunWe need to keep that, because it's essential information for how to apply HTML attributes to the LI tag. I've removed the
- ...:
part now, HTH.Comment #38
JacineThat's better, thanks. :)
Comment #39
catch#37: drupal8.theme-item-list.37.patch queued for re-testing.
Comment #40
catchOK this looks great to me but this comment made my eyes roll into the back of my head:
I think it's "nest child lists" then "child list attributes", and the fact that the sentence both begins and ends with the same two words.
Maybe 'lists with nested children', and 'attributes for the children'?
Comment #41
sunThanks! Completely rephrased that comment, hopefully for the better. ;)
Comment #42
tim.plunkettSmall grammar nitpick
s/inherited to/inherited by
Comment #43
sunUhm, also fixed that.
Comment #44
tim.plunkettCode was already RTBC, and the new comment is much better.
Comment #45
catchCommitted to 8.x.
This is a near complete refactoring of the theme function, so marking CNR for 7.x.
Comment #46
sun#43: drupal8.theme-item-list.43.patch queued for re-testing.
Comment #47
JacinePersonally, I think attempting to backport will be a waste of time. Same for #98696: Various bugs in theme_links().
Much smaller theme layer backports have been attempted and failed, so removing the tag and marking this fixed.