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>
Files: 
CommentFileSizeAuthor
#66 core-provide_theme_dl_simplified-54898-66.patch4.36 KBpsynaptic
PASSED: [[SimpleTest]]: [MySQL] 53,244 pass(es). View
#60 core-provide_theme_dl_simplified-54898-60.patch4.74 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 52,247 pass(es). View
#54 drupal8.theme-dl.54.patch7.93 KBsun
PASSED: [[SimpleTest]]: [MySQL] 46,228 pass(es). View
#54 theme-dl.png17.57 KBsun
#45 d8-theme-item-list-process-definition-lists-54898-45.patch4.06 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-theme-item-list-process-definition-lists-54898-45.patch. Unable to apply patch. See the log in the details link for more information. View
#45 d8-theme-item-list-process-definition-lists-example-output.png75.07 KBsteveoliver
#37 0001-Issue-54898-Let-theme_item_list-function-process-def.patch7.71 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 34,011 pass(es). View
#32 0001-Let-theme_list_items-function-process-definition-lis.patch7.36 KBmarvil07
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-Let-theme_list_items-function-process-definition-lis.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#30 0001-modify-theme_item_list-instead-of-adding-theme_defin.patch7.37 KBmarvil07
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-modify-theme_item_list-instead-of-adding-theme_defin.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#24 theme_definition_list-v7b.patch6.06 KBmarvil07
Unable to apply patch theme_definition_list-v7b.patch View
#22 theme_definition_list-v7.patch7.88 KBmarvil07
Passed on all environments. View
#20 theme_definition_list_screenshot.png9.55 KBsoxofaan
#19 theme_definition_list.inc_.txt2.63 KBsoxofaan
#9 dl-list.patch6.84 KBNick Lewis
Invalid patch format in dl-list.patch. View
#5 theme_definition_list.patch_0.txt5.09 KBBèr Kessels
#3 theme_definition_list.patch.txt1.64 KBBèr Kessels
#1 theme_list_dl_func_addition.patch1.18 KBBèr Kessels
Passed on all environments. View
theme_list_dl_addition.patch1.15 KBBèr Kessels
Unable to apply patch theme_list_dl_addition.patch View

Comments

Bèr Kessels’s picture

FileSize
1.18 KB
Passed on all environments. View

This 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.

Crell’s picture

Status: Needs review » Needs work

+1 on the concept. However, definition lists in HTML permit multiple dd's per dt. We should allow for that, too.

Bèr Kessels’s picture

Status: Needs work » Needs review
FileSize
1.64 KB

This version allows for multiple DDs.

Bèr Kessels’s picture

I also added this theme function to helpers http://drupal.org/node/62453, in case someone needs it now.

Bèr Kessels’s picture

This patch allows one to override the filters, that were hardcoded in the previous patch. Usefull for full HTML definitions.

drumm’s picture

- There is a double ;-ed line.
- I would like to see extra wrapper divs 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})?

drumm’s picture

Status: Needs review » Needs work
m3avrck’s picture

Reviving this feature for another look, would be very handy.

Nick Lewis’s picture

FileSize
6.84 KB
Invalid patch format in dl-list.patch. View

I'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:

  $items = array(
    // set id (optional) for themers
    'id' => 'example',
    'attributes' => $attributes, 
    // definitions are childern of terms for programming reasons (differs from markup)
    'terms' => array(
      'term 1' => array(
        'value' => 'term 1 title',
        // the href key is weird, but needed for definition lists built from nodes and menus
        'href' => 'http://localhost.com/term1.html',
        'attributes' => array('class' => 'term1class'),
        'definitions' => array(
          'd1' => array('value' => 'definition 1', 'attributes' => array('id' => 'def1')),
          'd2' => array('value' => 'definition 2', 'attributes' => array('id' => 'def2')),
        )
      )
    
    )
  
  );

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.)

Nick Lewis’s picture

Title: Add definition list feature to theme_item_list » Add theme_definition_list($items) to theme.inc

more 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.

Nick Lewis’s picture

Status: Needs work » Needs review

There'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.

Bèr Kessels’s picture

Good 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.

Nick Lewis’s picture

Okay, 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:

switch($lists['id']) {
  case 'admin-lists':
    foreach ($terms as $key => $value) {
      switch($key) {
         case 'site building': 
            $term[$key]['attributes'] = array('class' => 'some background image thingy');   
            break;
     } 
     break;
}

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.

coreb’s picture

Version: x.y.z » 6.x-dev
Status: Needs review » Patch (to be ported)

Moving out of the "x.y.z" queue to a real queue.

coreb’s picture

Status: Patch (to be ported) » Needs review

I incorrectly used the new "patch (to be ported)" status.

solipsist’s picture

Ah bugger, wish I'd seen this sooner! It should have made it into D6!

Gábor Hojtsy’s picture

Bèr's themeing claims still stand. The 'documentation comment' in the theme function also looks out of place.

Nick Lewis’s picture

Status: Needs review » Needs work

Having 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.

soxofaan’s picture

I'd also like to chime in with a theme_definition_list() function (based as much as possible on theme_item_list() )

highlights, features, properties:

  • no filter argument like in the patches of Bèr Kessels to mimic theme_item_list()
  • $attribute argument to mimic theme_item_list()
  • nested definition lists are possible
  • keys in array are used as terms as suggested by drummm in #6
  • term string from array key can be overriden
  • input $items can be as simple as array("apple" => "round", "banana" => "yellow")
  • input $items can be more complex to add special attributes to the desired dl, dt or dd element
soxofaan’s picture

follow up of my post at #19
example input $items array:

$items = array();
$items['Drupal'] = 'kills cute little puppies';
$items['zzz'] = array(
  'term' => 'Insert more meaningfull title than "zzz" here',
  'attributes' => array('style' => 'color: blue;'),
  'definition 1 of zzz',
  array(
    'data' => 'definition 2 of zzz with a nested definition list:',
    'attributes' => array('style' => 'color: red;'),
    'children' => array('apple'=> 'round', 'banana' => 'yellow'),
  ),
  'definition 3 foo bar baz foz zzz',
);

$output .= theme('definition_list', $items);

screenshot of result in attachment

NancyDru’s picture

Version: 6.x-dev » 7.x-dev
marvil07’s picture

Assigned: Bèr Kessels » Unassigned
Category: task » feature
Status: Needs work » Needs review
FileSize
7.88 KB
Passed on all environments. View

hey, 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

sun’s picture

Status: Needs review » Needs work

We want to inline this code into theme_item_list(). Add special support for $key == 'term' and you're basically set.

marvil07’s picture

Status: Needs work » Needs review
FileSize
6.06 KB
Unable to apply patch theme_definition_list-v7b.patch View

so, 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...

Status: Needs review » Needs work

The last submitted patch failed testing.

Crell’s picture

I 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.

marvil07’s picture

Status: Needs work » Needs review

@Crell: ok, so patch in #22 seems better?

effulgentsia’s picture

@sun: Care to respond to #26? Is a refinement of either #22 or #24 still in-scope for D7?

webchick’s picture

Version: 7.x-dev » 8.x-dev

No more API changes in 7.x.

marvil07’s picture

FileSize
7.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-modify-theme_item_list-instead-of-adding-theme_defin.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Updating the patch :-)

klausi’s picture

Status: Needs review » Needs work

patch does not apply anymore

marvil07’s picture

Title: Add theme_definition_list($items) to theme.inc » Let theme_list_items function process definition lists
Status: Needs work » Needs review
FileSize
7.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-Let-theme_list_items-function-process-definition-lis.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

To synchronize title with patch, I've just changed the title.

Patch now applies on top of 8.x

NancyDru’s picture

Angie, even Dries agreed that if the change does not affect current APIs, then it is possible to backport an API addition.

webchick’s picture

Issue tags: +needs backport to D7

Fixing tag.

wojtha’s picture

Status: Needs review » Needs work
Issue tags: +needs backport to D7

The last submitted patch, 0001-Let-theme_list_items-function-process-definition-lis.patch, failed testing.

marvil07’s picture

Title: Let theme_list_items function process definition lists » Let theme_item_list function process definition lists
Status: Needs work » Needs review
FileSize
7.71 KB
PASSED: [[SimpleTest]]: [MySQL] 34,011 pass(es). View

There was some changes on theme_item_list() since I last updated this patch. So here again updated to 8.x branch.

BarisW’s picture

Yes please! Awesome to have this in core.

psynaptic’s picture

Anyone know if this is still being considered for Drupal 8?

psynaptic’s picture

I'm not against the idea of a separate twig template for definition lists.

steveoliver’s picture

Status: Needs review » Needs work
Issue tags: +theme system cleanup

Tagging 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') to theme('list').

steveoliver’s picture

To 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?

jwilson3’s picture

Tagging.

steveoliver’s picture

Assigned: Unassigned » steveoliver

So 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.

steveoliver’s picture

FileSize
75.07 KB
4.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-theme-item-list-process-definition-lists-54898-45.patch. Unable to apply patch. See the log in the details link for more information. View

First, 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

  1. Definition lists are interesting. A <dl> consists of any number of <dt> terms optionally followed by any number of <dd> definitions.
  2. DL Examples come from the w3 html4.01 spec for definition lists
  3. Code to make this work is found by applying the patch in this comment to the latest front-end branch of the D8 Twig Theme Sprint repo.
  4. In this patch, <dd> elements do not support children or attributes -- I want to get feedback with the least amount of code in the way...
  5. A single template and preprocess function are used to support both list types (<li> and <dt>/<dd>. This allows (after a little @todo work) any combination of nested lists.
  6. Look at the implementation (in the lists_example.module file the 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?
  7. Hardly anything is different in _preprocess.

API Changes

  1. dl is a supported type
  2. dt and dd are supported properties of each item in items.

Implementation

  $output = '';

  // Simple Definition List
  $type = 'dl';
  $title = 'Simple Definition List';
  $attributes = array();
  $items = array();
  $items[] = array(
    'dt' => array('dweeb'),
    'dd' => array('young excitable person who may mature into a Nerd or Geek'),
  );
  $items[] = array(
    'dt' => array('hacker'),
    'dd' => array('a clever programmer'),
  );
  $items[] = array(
    'dt' => array('nerd'),
    'dd' => array('technically bright but socially inept person'),
  );
  $items[] = array(
    'dt' => array(
      'Center',
      'Centre',
    ),
    'dd' => array(
      'A point equidistant from all points on the surface of a sphere.',
      'In some field sports, the player who holds the middle position on the field, court, or forward line.'
    ),
  );
  $output .= theme('item_list', array(
    'title' => $title,
    'type' => $type,
    'items' => $items,
    'attributes' => $attributes,
  ));

  // Mixed Lists.
  $type = 'ol';
  $title = 'Mixed Lists';
  $attributes = array(
    'id' => 'top-level-list-id',
  );
  $items = array();
  $items[] = 'top level item - 1';
  $items[] = 'top level item - 2';
  $items[] = array(
    'id' => 'top-level-item-3-id',
    'data' => 'top level item - 3',
    'children' => array(
      'second level item - 3.1',
      'second level item - 3.2',
      array(
        'class' => array('second-level-item-3-3-class'),
        'test-attribute' => 'test-attribute-value',
        'id' => 'second-level-item-3-3-id',
        'data' => 'second level item - 3.3',
        'type' => 'ul',
        'children' => array(
          'third level item - 3.3.1',
          'third level item - 3.3.2',
          array(
            'data' => 'third level item - 3.3.3',
            'class' => array('third-level-item-3-3-3-class'),
            'id' => 'third-level-item-3-3-3-id',
            'type' => 'ol',
            'children' => array(
              'fourth level item - 3.3.3.1',
              'fourth level item - 3.3.3.2',
            ),
          ),
          array(
            'data' => 'third level item - 3.3.4',
            'class' => array('third-level-item-3-3-4-class'),
            'id' => 'third-level-item-3-3-4-id',
            'children' => array(
              array(
                'data' => 'fourth level item - 3.3.4.1, showing you a definition list:',
                'type' => 'dl',
                'class' => 'fourth-level-definition-list-class',
                'id' => 'fourth-level-definition-list-id',
                'children' => array(
                  // Each is an item.
                  array(
                    'dt' => array('dweeb'),
                    'dd' => array('young excitable person who may mature into a Nerd or Geek'),
                  ),
                  array(
                    'dt' => array('hacker'),
                    'dd' => array('a clever programmer'),
                  ),
                  array(
                    'dt' => array('nerd'),
                    'dd' => array('technically bright but socially inept person'),
                  ),
                  array(
                    'dt' => array(
                      'Center',
                      'Centre',
                    ),
                    'dd' => array(
                      'A point equidistant from all points on the surface of a sphere.',
                      'In some field sports, the player who holds the middle position on the field, court, or forward line.'
                    ),
                  ),
                ),
              ),
            ),
          ),
        ),
      ),
    ),
  );
  $output .= theme('item_list', array(
    'items' => $items,
    'title' => $title,
    'type' => $type,
    'attributes' => $attributes
  ));

  return $output;

Here is the rendered html (screenshot):
d8-theme-item-list-process-definition-lists-example-output.png

And here is the html markup:

  <div class="item-list">
    <h3>Simple Definition List</h3>

    <dl>
      <dt>dweeb</dt>

      <dd>young excitable person who may mature into a <em>Nerd</em> or
      <em>Geek</em></dd>

      <dt>hacker</dt>

      <dd>a clever programmer</dd>

      <dt>nerd</dt>

      <dd>technically bright but socially inept person</dd>

      <dt>Center</dt>

      <dt>Centre</dt>

      <dd>A point equidistant from all points on the surface of a sphere.</dd>

      <dd>In some field sports, the player who holds the middle position on the field,
      court, or forward line.</dd>
    </dl>
  </div>

  <div class="item-list">
    <h3>Mixed Lists</h3>

    <ol id="top-level-list-id">
      <li>top level item - 1</li>

      <li>top level item - 2</li>

      <li id="top-level-item-3-id">top level item - 3

        <div class="item-list">
          <ol>
            <li>second level item - 3.1</li>

            <li>second level item - 3.2</li>

            <li class="second-level-item-3-3-class" test-attribute="test-attribute-value"
            id="second-level-item-3-3-id">second level item - 3.3

              <div class="item-list">
                <ul>
                  <li>third level item - 3.3.1</li>

                  <li>third level item - 3.3.2</li>

                  <li class="third-level-item-3-3-3-class" id=
                  "third-level-item-3-3-3-id">third level item - 3.3.3

                    <div class="item-list">
                      <ol>
                        <li>fourth level item - 3.3.3.1</li>

                        <li>fourth level item - 3.3.3.2</li>
                      </ol>
                    </div>
                  </li>

                  <li class="third-level-item-3-3-4-class" id=
                  "third-level-item-3-3-4-id">third level item - 3.3.4

                    <div class="item-list">
                      <ul>
                        <li class="fourth-level-definition-list-class" id=
                        "fourth-level-definition-list-id">fourth level item - 3.3.4.1,
                        showing you a definition list:

                          <div class="item-list">
                            <dl>
                              <dt>dweeb</dt>

                              <dd>young excitable person who may mature into a
                              <em>Nerd</em> or <em>Geek</em></dd>

                              <dt>hacker</dt>

                              <dd>a clever programmer</dd>

                              <dt>nerd</dt>

                              <dd>technically bright but socially inept person</dd>

                              <dt>Center</dt>

                              <dt>Centre</dt>

                              <dd>A point equidistant from all points on the surface of a
                              sphere.</dd>

                              <dd>In some field sports, the player who holds the middle
                              position on the field, court, or forward line.</dd>
                            </dl>
                          </div>
                        </li>
                      </ul>
                    </div>
                  </li>
                </ul>
              </div>
            </li>
          </ol>
        </div>
      </li>
    </ol>
  </div>
sun’s picture

Meanwhile, 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.

steveoliver’s picture

@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.

jwilson3’s picture

Could 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

jwilson3’s picture

Or 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. :)

steveoliver’s picture

It 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.

Crell’s picture

I said back in #26 that definition lists should be a separate theme call from ul/ol lists. I still stand by that statement. :-)

sun’s picture

Title: Let theme_item_list function process definition lists » Add a theme_definition_list() function to theme a definition list
Status: Needs work » Active
Issue tags: -needs backport to D7, -theme system cleanup, -Theme Component Library

Renaming 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.

psynaptic’s picture

I agree with the recent comments. Let's have a separate theme component.

sun’s picture

Title: Add a theme_definition_list() function to theme a definition list » Add a theme_description_list() function to theme a description list (ex. definition list)
Assigned: steveoliver » Unassigned
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
17.57 KB
7.93 KB
PASSED: [[SimpleTest]]: [MySQL] 46,228 pass(es). View

A "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:

 * "The dl element represents an association list consisting of zero or more
 * name-value groups (a description list). Each group must consist of one or
 * more names (dt elements) followed by one or more values (dd elements).
 * Within a single dl element, there should not be more than one dt element for
 * each name."
 *
 * @see http://html5doctor.com/the-dl-element/ 
 * @see http://www.w3.org/TR/html-markup/dl.html 
 * @see http://www.w3.org/wiki/HTML_lists#Description_lists 
 *
 * This means:
 * - The DL may be empty.
 * - If there is a DT, it must be followed by a DD.
 * - There can be multiple DTs followed by one or more DDs.
 * - Each set of DTs and DDs forms a group.
 * - The text of one DT must be unique within the DL.
 * - The DL must contain DT and DD elements only.

I've added an example implementation directly to the default front page for manual testing, which works as expected:

theme-dl.png

Speaking of "expected", that needs to be turned into a test.

sun’s picture

btw, 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.

webchick’s picture

Mmmm. Ice cream.

sun’s picture

Issue tags: +Needs themer review
jwilson3’s picture

Couple things:

  1. Since "definition lists" are now "description lists" in HTML5, provide an option to specifically denote when a term is a definition.
    <dl>
        <dt><dfn>Term</dfn></dt>
        <dd>Description</dd>
    </dl>
    
  2. Show an example of proper usage of Attributes in the node.module examples. I might suggest the color/colour example from the HTML5Doctor site.
    <dl>
      <dt lang="en-GB"><dfn>colour</dfn></dt>
      <dt lang="en-US"><dfn>color</dfn></dt>
      <dd>The visual result of light in their emission, transmission and/or reflection. This perception is determined by the hue, brightness and saturation of the light at a specific point. </dd>
    </dl>
    
  3. Add an option, defaulting to TRUE, that would coalesce multiple instances of the same term(s) into a single term (e.g. using a hashtable system in the preprocess function) to follow the W3C. Given that there can be multiple terms (synonyms), this could be an interesting code exercise.
c4rl’s picture

Issue tags: +theme system cleanup

Tagging

jenlampton’s picture

Category: feature » task
FileSize
4.74 KB
PASSED: [[SimpleTest]]: [MySQL] 52,247 pass(es). View

Rerolled 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.

sun’s picture

Status: Needs review » Needs work

Well, 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.

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.

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.

jenlampton’s picture

The 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.

psynaptic’s picture

I 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.

psynaptic’s picture

Assigned: Unassigned » psynaptic
jenlampton’s picture

Yeah, 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

psynaptic’s picture

Status: Needs work » Needs review
FileSize
4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 53,244 pass(es). View

I 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

steveoliver’s picture

Status: Needs review » Needs work
Issue tags: -Needs themer review

This 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.

mitokens’s picture

joelpittet’s picture

Title: Add a theme_description_list() function to theme a description list (ex. definition list) » Add a description-list.html.twig template (ex. definition list)
Version: 8.0.x-dev » 8.1.x-dev
Assigned: psynaptic » Unassigned
Issue tags: +Twig

If 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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jeff Burnz’s picture

Real shame we still have to hard code these lists, btw #69 https://www.drupal.org/project/definition_list

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.