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.
This patch adds a DT to the theme function.
$items = array(
array("term" => "A", "definition" => "letter A"),
array("term" => "B", "definition" => "letter B"),
array("term" => "C", "definition" => "letter C"),
);
print htmlentities(theme('item_list', $items, "The alphabet", 'dl'));
Will now return a nice DL list:
<div class="item-list"><h3>The alphabet</h3><dl><dt>A</dt><dd>letter A</dd><dt>B</dt><dd>letter B</dd><dt>C</dt><dd>letter C</dd></dl></div>
Comment | File | Size | Author |
---|---|---|---|
#83 | core-provide_theme_dl_simplified-54898-83.patch | 3.92 KB | Martijn de Wit |
#54 | drupal8.theme-dl.54.patch | 7.93 KB | sun |
#54 | theme-dl.png | 17.57 KB | sun |
#45 | d8-theme-item-list-process-definition-lists-54898-45.patch | 4.06 KB | steveoliver |
#45 | d8-theme-item-list-process-definition-lists-example-output.png | 75.07 KB | steveoliver |
Comments
Comment #1
Bèr Kessels CreditAttribution: Bèr Kessels commentedThis is an alternative. Here we introduce a new function. theme_item_list refers to item lists. A DL is not really such a thing.
Therefore, this patch introduces a
theme_definition_list
.Comment #2
Crell CreditAttribution: Crell commented+1 on the concept. However, definition lists in HTML permit multiple dd's per dt. We should allow for that, too.
Comment #3
Bèr Kessels CreditAttribution: Bèr Kessels commentedThis version allows for multiple DDs.
Comment #4
Bèr Kessels CreditAttribution: Bèr Kessels commentedI also added this theme function to helpers http://drupal.org/node/62453, in case someone needs it now.
Comment #5
Bèr Kessels CreditAttribution: Bèr Kessels commentedThis patch allows one to override the filters, that were hardcoded in the previous patch. Usefull for full HTML definitions.
Comment #6
drumm- There is a double ;-ed line.
- I would like to see extra wrapper
div
s avoided.- There is a deifnition list in node.module, and maybe more elsewhere, I would like to see at least one rewritten to use this to simplify code a provide an example of how useful this is.
- Why not use
array(term => {string defintion|array definitions})
?Comment #7
drummComment #8
m3avrck CreditAttribution: m3avrck commentedReviving this feature for another look, would be very handy.
Comment #9
Nick Lewis CreditAttribution: Nick Lewis commentedI've taken a different approach. The problem with the one above is that it doesn't allow multiple definitions, and isn't quite as themable. I'm not sure if this approach is unnecessarily ugly, so I'd love some feedback. Here's an example array that would create a definition list. I shows the flexibility, albeit complexity:
The patch contains a new theme function, and redoes the node, and administration definiton lists.
I'd love some feedback -- and if you think this approach is reasonable, I'll submit a full patch (I think I forgot a few definitin lists.)
Comment #10
Nick Lewis CreditAttribution: Nick Lewis commentedmore descriptive title -- and I'll remind you this is an important features, since *ALL* of 5.0's root admin menus are essentially definition lists. Thank you for your time. God bless.
Comment #11
Nick Lewis CreditAttribution: Nick Lewis commentedThere's a lot of feature requests on theming the node/add page, and the administration pages that this will take care of. I guess I'll change this to review.
Comment #12
Bèr Kessels CreditAttribution: Bèr Kessels commentedGood that you took this up nick.
However, the 'theme' function as you propose in your last patch is horrible. Not from PHP or code-p-o-v. But fur themers. It is *far* too complex, contains far too much algorithms and it looks scary for people not big into coding.
First aim of theme functions should be to *hide* complexity. (note, that Nicks approach is even better and simpler then my first attempt, I am only trying to make this even better)
* Drop the
if (!empty($items)) {
. Force the people calling the theme function to giove good input. LEss logic in a theme function == better.* optionally: split up into three functions: theme_definition_list, theme_definition_term, theme_definition_definition. My experience is that themers/desigers grok that much better then nested foreach and for loops.
But, all in all, I really dislike the nested array approach, it only makes stuff more complex.
* why not pass things as "id" or "terms" as separate parameters, they ahave absolutely nothing to do with eachother, so they don't elong no one array?
* why all the nesting? The specs aer clear: A DL can have any amout of DD and DTs, but only one level deep. I think that fact can simplify the code a lot.
Comment #13
Nick Lewis CreditAttribution: Nick Lewis commentedOkay, I'll bite:
Yeah.... I'm not particularly happy with it either... but... its really not that hard to deal with. Its a lot easier for people who might want to do Say you're doing a phptemplate override. You may want to add images to a certain definition list, say the one on the admin page. Now let's say that you want to add an image that floats left beside the term's definition. Using this system, I could go:
If I didn't put defintions as children of a term, it would create quite a bit of difficulty in sharp shooting one certain definition in a list... I don't know, I see your point about simplicity, but I don't see how not giving themes attributes, and keys => value to screw with helps them. I also think its im[portant to be able to specify their own keys in cases where they might want to alter the structure of the array (e.g. add an image key) for better rendering...
Maybe I'm missing something obvious.
Comment #14
coreb CreditAttribution: coreb commentedMoving out of the "x.y.z" queue to a real queue.
Comment #15
coreb CreditAttribution: coreb commentedI incorrectly used the new "patch (to be ported)" status.
Comment #16
solipsist CreditAttribution: solipsist commentedAh bugger, wish I'd seen this sooner! It should have made it into D6!
Comment #17
Gábor HojtsyBèr's themeing claims still stand. The 'documentation comment' in the theme function also looks out of place.
Comment #18
Nick Lewis CreditAttribution: Nick Lewis commentedHaving only been programmer for 2 years, 7 months makes a big difference in terms of my sensibilities. My solution is weird... and I don't like it personally.
As far as the alternative proposed by Ber, deep down, I still think DL lists are going to be used for complex information, and that no two definition list use cases are going to be anything alike. If you cannot selectively override the instance (which is the main admin page, and node add page in drupal core), there's little value to this patch, imho. Simplicity can make things more complex, for example, "simplicity" forces themers to always have to do moves like if (arg(0) == admin) (when I use arg(x) in phptemplate overrides, frankly, I want to set buildings on fire.....)
Having the ability to pass ids and classes is a must have... and if we do, lets just use the same method we use in links, formapi, item lists, and everywhere else: drupal_attributes().
Give average themer some credit. IMHO, a semi-competent themer will have learned the how to "print_r($definition_list) ". If they don't know know what print_r is, than the bigger question is should we be designing our functions for people like that in the first place. If you know "print_r()", than its much easier than say, the anti-patterns of the l(), theme_item_list(), theme_links() functions which seperate parameters into odd orders that must be memorized, when one variable as an array would have done fine.
Just my opinion, but i think the entire value in this function is encouraging use of the very useful html element type, and opening up the admin overview page, and node add page to themers in a much more granular fashion than is currently possible.
I think altogether the code needs work... anyone can override my opinion.
Comment #19
soxofaan CreditAttribution: soxofaan commentedI'd also like to chime in with a theme_definition_list() function (based as much as possible on theme_item_list() )
highlights, features, properties:
array("apple" => "round", "banana" => "yellow")
Comment #20
soxofaan CreditAttribution: soxofaan commentedfollow up of my post at #19
example input $items array:
screenshot of result in attachment
Comment #21
NancyDruComment #22
marvil07 CreditAttribution: marvil07 commentedhey, I reworked(way passing variables changed since then) the lasted attach to make it a real patch against D7, and also included an example in the node module. Specially now that we're using definition lists to make hook_help
Comment #23
sunWe want to inline this code into theme_item_list(). Add special support for $key == 'term' and you're basically set.
Comment #24
marvil07 CreditAttribution: marvil07 commentedso, this patch tries to do it from theme_item_list instead of making a new theme function.
IMHO this:
- reduces the added code
- make it a little less "intuitive" the way of adding DLs, but also make it near to the standard idea(DTs and DLs are at the same level)
for the record, the official reference: http://www.w3.org/TR/xhtml-modularization/abstract_modules.html#s_listmo...
Comment #26
Crell CreditAttribution: Crell commentedI disagree. Definition lists, despite the word "list", are structurally quite different than ul/ol lists. It should be a separate function since the API is so different.
Comment #27
marvil07 CreditAttribution: marvil07 commented@Crell: ok, so patch in #22 seems better?
Comment #28
effulgentsia CreditAttribution: effulgentsia commented@sun: Care to respond to #26? Is a refinement of either #22 or #24 still in-scope for D7?
Comment #29
webchickNo more API changes in 7.x.
Comment #30
marvil07 CreditAttribution: marvil07 commentedUpdating the patch :-)
Comment #31
klausipatch does not apply anymore
Comment #32
marvil07 CreditAttribution: marvil07 commentedTo synchronize title with patch, I've just changed the title.
Patch now applies on top of 8.x
Comment #33
NancyDruAngie, even Dries agreed that if the change does not affect current APIs, then it is possible to backport an API addition.
Comment #34
webchickFixing tag.
Comment #35
wojtha CreditAttribution: wojtha commented#32: 0001-Let-theme_list_items-function-process-definition-lis.patch queued for re-testing.
Comment #37
marvil07 CreditAttribution: marvil07 commentedThere was some changes on
theme_item_list()
since I last updated this patch. So here again updated to 8.x branch.Comment #38
BarisW CreditAttribution: BarisW commentedYes please! Awesome to have this in core.
Comment #39
psynaptic CreditAttribution: psynaptic commentedAnyone know if this is still being considered for Drupal 8?
Comment #40
psynaptic CreditAttribution: psynaptic commentedI'm not against the idea of a separate twig template for definition lists.
Comment #41
steveoliver CreditAttribution: steveoliver commentedTagging and setting to 'needs work'.
Cross-posted in #1484720: [Meta] Reduce the number of theme functions/templates.
In order to support definition lists, I like the idea of converting
theme('item_list')
totheme('list')
.Comment #42
steveoliver CreditAttribution: steveoliver commentedTo be clear: The case for making theme('
item_list') support definition lists is when we want a single list including all types (i.e.ol > li > dt > dd > ul > li
.Otherwise, we just have theme('item_list') and theme('definition_list'). Which makes it a little difficult to nest flexibly.
Right?
Comment #43
jwilson3Tagging.
Comment #44
steveoliver CreditAttribution: steveoliver commentedSo in working on getting this working in the front-end branch of pixelmord's D8 Twig Theme Sprint sandbox. It looks like item.definitions will be the only new API change if this works as expected. Using template suggestions like list--definition may or may not be necessary or helpful. Patch coming up, will post in Twig queue and cross-link back over here. So far, it's not looking too bad. Assigning to myself, too.
Comment #45
steveoliver CreditAttribution: steveoliver commentedFirst, my apologies if anyone is uncomfortable looking at the template (in the patch) in Twig syntax. It's what I've been using to work on item-list for the D8 Twig sprint.
Now..
This patch applies to the current HEAD of front-end branch of Pixelmord's D8TTS repo.
I didn't post it in the other issue I mentioned (#1800726: Convert theme_item_list to item-list.twig) as this is the actual issue for this feature.
Notes
<dl>
consists of any number of<dt>
terms optionally followed by any number of<dd>
definitions.<dd>
elements do not supportchildren
orattributes
-- I want to get feedback with the least amount of code in the way...<li>
and<dt>/<dd>
. This allows (after a little @todo work) any combination of nested lists.the lists_example.module filethe code below)... Should we expect this to be a common use case (nested lists of different types)? Or should we expect that child lists of different types to be rendered beforehand into the 'data' of an item?_preprocess
.API Changes
dl
is a supportedtype
dt
anddd
are supported properties of each item initems
.Implementation
Here is the rendered html (screenshot):
And here is the html markup:
Comment #46
sunMeanwhile, I'm not at all sure whether this is a good idea. Actually rather against it.
UL/OL lists do not have a term/description separation, and the semantic specification of definition lists is different/special.
The DL/DT/DD triple requires different processing than *L/LI doubles.
I think we want a theme function for definition lists in core, but I don't think we should (ab)use the list function.
Comment #47
steveoliver CreditAttribution: steveoliver commented@sun, Agree.
After going through the exercise (again, in order to support a single list of different nested child types), I'm thinking the use case for this mix of case is rather small while the uglification of item_list handling is large :).
Additionally, theme('definition_list') would have a simpler API as it would likely not be dancing around 'data' but instead support 'terms' and 'definitions' or some such keys.
Comment #48
jwilson3Could the theme function be separate, but still allow theme_list to accept type of dl and hand it off to the separate function for processing and rendering... that code example you added is pretty convincing and elegant tho the underpinnings are complex
Comment #49
jwilson3Or better yet, just abstract it out into a separate theme suggestion, and companion preprocess theme_list__definition? Shot in the dark, feel free to shoot me down. :)
Comment #50
steveoliver CreditAttribution: steveoliver commentedIt could be done, and it's tempting to try it -- as you can see from all of our attempts above, but why?... sun in #46 pretty much nails it.
I'd say we create theme('definition_list') in D8 and someone can always make a dl.module for D7 contrib ;)
when we have a definition list we want in another list, render the dl first, and pass it as 'data' into item_list, or as a dd to another dl.
Comment #51
Crell CreditAttribution: Crell commentedI said back in #26 that definition lists should be a separate theme call from ul/ol lists. I still stand by that statement. :-)
Comment #52
sunRenaming and re-classifying. I was 99% sure there was a separate issue that only asked for a new function, but I searched extensively and couldn't find one, so let's re-purpose this one here.
Comment #53
psynaptic CreditAttribution: psynaptic commentedI agree with the recent comments. Let's have a separate theme component.
Comment #54
sunA "quickly" wanted to kick-start this, only to realize that the specification of description lists is not exactly trivial to implement as a theme function. (HTML5 no longer calls this a definition list)
Attached patch is what I came up with, following clean-up and simplification practices we're currently doing elsewhere.
The most important part is this:
I've added an example implementation directly to the default front page for manual testing, which works as expected:
Speaking of "expected", that needs to be turned into a test.
Comment #55
sunbtw, I was tempted to rename the theme function to just 'dl' (instead of 'description_list') and also the array keys in groups to just 'dt' and 'dd' (instead of 'term' and 'description'). But didn't do so, since we are using these descriptive identifiers/keys in all other theme functions. Nevertheless, something to consider.
Comment #56
webchickMmmm. Ice cream.
Comment #57
sunComment #58
jwilson3Couple things:
Comment #59
c4rl CreditAttribution: c4rl commentedTagging
Comment #60
jenlamptonRerolled with @sun's comments from #55 for a few reasons.
We'll soon have a ol.html.twig file and ul.html.twig file so a dt.html.twig is consistent.
More intuitive names for things in the template files is better (and removing the key simplifies the code and reduces complexity!)
I also removed the extra div around the dl tag, as well as the title, since we'll be removing the div & title from item lists as well.
This new patch is also overly simplified (removing preprocess, and assuming all calls to DL will be only one level deep), since I have yet to find an example core with a DL that is more than one level deep. One of our principles for the new theme system is not to build something we don't have a use-case for, so unless someone can point me to a place in core where we need nested DLs, I'm tempted to leave that out entirely.
Comment #61
sunWell, this does not follow the HTML spec. There can be multiple DTs for a DD and multiple DDs for a DT. See #54.
If we don't implement the spec, then this is cannot be used for many use-cases. I strongly disagree with that.
You also removed support for render arrays as values, which I disagree with, too.
For DLs, it can probably be omitted, yes. But where was/is the removal of title discussed for item lists? I don't see the point of that, so I'd like to understand and verify the reasons.
Comment #62
jenlamptonThe UL and OL HTML elements do not contain a title, so neither should our item-list templates. The basic concept is that we need to organize Drupal's front end more like HTML does - in the *same* ways that front-end devs expect to find them. (This was discussed and decided at the BADCamp Twig sprint, and is documented in our principles.) These changes also includes things like not using a single template to render both a UL and a OL, since they are *not* the same thing semantically, and not separating out a piece of a HTML element into a separate template (like theme_menu_tree vs theme_menu_item).
As for leaving in the additional complexity, I think matching the HTML spec is the perfect reason. Even if we don't ever use it in core, the long-term goal is that contrib will be able to use these templates instead of creating their own, and someday someone might want some of the additional complexity.
Comment #63
psynaptic CreditAttribution: psynaptic commentedI like that item lists have titles as quite often you need a title to keep the semantic meaning correct in the context of the document as a whole. I don't mind if we remove it from ul/ol/dl but we'd have to provide some way to print a list with a title and wrapper.
Comment #64
psynaptic CreditAttribution: psynaptic commentedComment #65
jenlamptonYeah, we'll have a generic container theme function that lets you wrap anything in a div and give it a title. That should be reused as much as possible, and will allow us to keep the list, just a list.
For removing the title and the extra div tag on item_lists, see #1842140: Remove title and wrapper div from item-list.html.twig
Comment #66
psynaptic CreditAttribution: psynaptic commentedI think sun was basically spot on with his patch. I've cleaned it up a bit and structured it in such a way to make the conversion to twig as easy as possible. This will change to a template_preprocess function so don't worry about the removal of that as it will be coming back in the conversion.
I created a test module and removed the testing code from the patch to save a step before this can be considered ready to be committed:
https://github.com/psynaptic/test_dl
Comment #67
steveoliver CreditAttribution: steveoliver commentedThis looks great, but could still use some tests:
- The dl element may be empty.
- If there is a dt element, it must be followed by a dd element.
- There can be multiple dt elements followed by one or more dd element.
- Each set of dt elements and dd elements forms a "group".
- The text of one dt element must be unique within the dl element.
- The dl element must contain dt and dd elements only.
Considering myself enough of a themer (to whom this makes enough sense), I'm removing the Needs themer review tag.
Comment #68
mitokens CreditAttribution: mitokens as a volunteer commentedComment #69
joelpittetIf this still has some use it could be created in contrib maybe but I'll move this to D8.1.x if someone wants to get this in as a feature. It needs to be converted to Twig. It would be great if we are building something like this that it is a #type even and that the tags are explicitly written dt and dd, not variables.
Points in #67 should still be addressed
Comment #72
Jeff Burnz CreditAttribution: Jeff Burnz commentedReal shame we still have to hard code these lists, btw #69 https://www.drupal.org/project/definition_list
Comment #77
Ambient.ImpactHi there. I saw that no one had updated this issue in ages and I had a need for description lists, so I thought I'd share the Twig template I put together. I'm still fairly new to Twig and Drupal 8 (been on 7 for ages), so feel free to give me feedback on this. I'd love it if this made it into core.
Edit: you can also view the current version in my repository.
Comment #78
opdaviesLet's see if we can review this within our contribution day today.
Comment #79
Ambient.ImpactThanks. In hindsight, a lot of the work in that template should probably be done in a pre-render method. 🤔 I may end up refactoring it.
Comment #82
Martijn de WitCreated a new patch based on #66 for Drupal > 8.8.x.
There are still no test in it... #67
Comment #83
Martijn de WitNew Try. Changed the old array code standard. Forgot a few lines to copy from old patch.
Still there also have to come a twig template & some tests.
Comment #84
Martijn de Wit