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.Meta issue: #1980004: [meta] Creating Dream Markup
Issue based on: #1898038: custom_block.module - Convert theme_ functions to Twig
Current markup proposal:
<nav role="navigation">
<ul class="nav-list">
{% for type in types %}
<li>
<a href="type.link.href">
<h2 class="nav-list__title">{{ type.label }}</h2>
<div class="nav-list__description">{{ type.description }}</div>
</a>
</li>
{% endfor %}
</ul>
</nav>
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | markup_for-1982200-1.patch | 1.42 KB | alansaviolobo |
| #12 | drupal8.markup.1982200-12.patch | 563 bytes | martin_q |
| #11 | twig-custom-block-markup-1982200-11.patch | 925 bytes | martin_q |
Comments
Comment #1
oresh CreditAttribution: oresh commentedmoving issue to core.
Comment #2
Hydra CreditAttribution: Hydra commentedThis should probbably be something like "custom-block-type-list"
Comment #3
webthingee CreditAttribution: webthingee commentedlatest patch is...
http://drupal.org/node/1898038
looks like...
Is there a reason this is node-type? It seems this should be block unless I am missing something. Not familiar with this module yet... is it displaying a node as a block (i.e. a node setting?)...
either...
or if it is a node
might fit what is needed here. As custom is the type of block, and list is the variation on the display.
Comment #4
ry5n CreditAttribution: ry5n commentedThere are a lot of problems with the class names here:
- With
class="node__block-custom--list"the new class naming conventions have been misapplied. This is neither an element within a node nor a special style of custom block (if it were a special style of custom block it would have to beclass="block-custom block-custom--list").- The bigger issue is that all of them, including
.block__custom--listand.node-type-listare missing the boat on modularity.Here’s what I’d do instead:
AFAIK this template renders a navigation list of block types to create, at /block/add, and as such is the same as all other admin navigation lists like /node/add or /admin. When I originally surveyed the Seven theme, this pattern was obvious, and I picked it out as the Nav List component. See http://groups.drupal.org/node/283223#Nav_List.
So, if admin navigation lists represent a reusable UI component, then:
1. I’m not sure why this module is using a custom template; can we not use theme_admin_block_content() [and also change the name to something more descriptive, like theme_nav_list()]?
2. Any default classes should reflect the component, not the specific content, so all we need is
class="nav-list".3. I am not sure we should be using a
<dl>. Although it’s semantically OK, it’s not the only valid way to mark this up, and we can’t wrap the dl/dt pairs if we need do. Also, AFAIK most screen readers still treat them as HMTL4 definition lists (if someone can correct me, I will be happy).This is one place where I think the Seven theme in D7 gets it right. This is hardly changed from D7:
Comment #5
stuajc CreditAttribution: stuajc commentedI agree that
node-type-listis an inappropriate class name, and it looks like it was just reused from the node add list at node/add. A quick fix would be changing that toblock-type-listand keeping all else the same, but as ry5n points out above, this is still not very modular. And possibly a bigger question is why use definition lists inconsistently throughout the admin interface to style these particular components?I like ry5n's proposed markup, which is much more semantic and reusable, but I would make one small change in line with the CSS Architecture page for D8, and remove the class on the
lis because it's kind of redundant and emphasizing the DOM structure. This is also in line with the menu system, which omits the "item" class.Comment #6
ry5n CreditAttribution: ry5n commentedI’m fine with no class on the
<li>. In many cases it’s not really necessary, although it does actually make the CSS *less* dependent on the HTML, but that’s not a big concern in this situation. So +1 for #5.Comment #7
jenlamptonCan someone please update the issue summary with the current proposed syntax?
Comment #8
martin_qSummary updated.
(Noob question) what is the next step and are we ready to do that now?
Comment #9
martin_qComment #10
martin_qWorking on a patch to implement this now.
Comment #11
martin_qThis patch implements the current proposed syntax.
Comment #12
martin_qAnd this separate patch corrects a grammatical error in custom_block.pages.inc, for full compliance with #1913208: [policy] Standardize template preprocess function documentation.
(Used the suggested filename this time, too.)
Comment #12.0
martin_qUpdated summary to reflect current state of thread.
Comment #13
alansaviolobo CreditAttribution: alansaviolobo commentedcombined and rerolled the 2 patches.
Comment #15
mgiffordThere has been no new work on this issue in quite some time. So I'm assuming the person assigned is no longer being actively pursuing it. Sincere apologies if this is wrong.