Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

FileSize
640 bytes

Simple change from using div on the wrapper to section

cosmicdreams’s picture

Status: Active » Needs review
andypost’s picture

+++ b/modules/taxonomy/taxonomy-term.tpl.phpundefined
@@ -38,7 +38,7 @@
+<section id="taxonomy-term-<?php print $term->tid; ?>" class="<?php print $classes; ?>">

maybe change id to class, some sites could use more then one term per page

1 days to next Drupal core point release.

cosmicdreams’s picture

@andypost The taxonomy-term-# id is already unique and therefore shouldn't cause an issue on sites that only have one term. Also, I think this code only renders on the page that appears when you click on the taxonomy term link. So, it's supposed to be the page that displays all content that has that taxonomy term associated to it.

I should also note that once rendered this code already has the class "taxonomy-term" so making the taxonomy-term-# id be the class would provide a small improvement. Some might say the improvement would be so small that it would be irrevelant.

All that said, I agree. My company has recently standardized on using classes instead of ids in most, if not all, cases. I think it's a powerful practice to implement application-wide as ids can become a crutch for patchy front-end development.

However, to implement this practice in drupal would require us to revisit a number of recently applied patches. I'm specifically talking about the comment-wrapper.tpl.php patch here. And I don't think the case has sufficiently been made yet why using classes instead of is a good idea for Drupal. If possible I'd like some time gather comments from the original authors of my company's front-end development best practices to make their case in their terms. But before I go to the effort, I should ask:

Are folks certain they want to include ids for major programatic elements of the template? May I make the case why we should use classes instead?

* Modified for grammar *

Jeff Burnz’s picture

How about we leave the whole can-of-worms of removing ID's for a separate issue, the patch here looks good.

cosmicdreams’s picture

Sounds good Jeff. Can I get someone to review/test this patch?

I've tested it myself and it works for me.

andypost’s picture

@cosmicdreams I agree that ID is out of scope of this patch but raised this because I think about it.

changing DIV to SECTION works fine but I can't test this for IE 7 & 8, IE9 works fine

cosmicdreams’s picture

If you have IE9, then you can test IE8 and 7 easily. Just open the Developer Tools (F12) then switch the rendering engine to older browser types by clicking on the "Browser Mode: IE9" at the top of the DevTool's toolbar.

Jeff Burnz’s picture

You need the shiv to test in < IE9, thats the issue, just add the shiv, its gonna break without it if any styling is applied to that wrapper. I think we can assume we going to have a shiv in core, one way or another. This patch is a no brainer imo, but waiting for Jacines feedback first.

andypost’s picture

FileSize
12.65 KB
4.94 KB
6.82 KB

Render looks different but does not different from current DIV implementation

So +1 to commit

Jacine’s picture

Issue tags: +D8H5

Tagging for this sprint.

Jacine’s picture

Whoops. It's not September. LOL.

dcmouyard’s picture

The only place I see this output is on a taxonomy term listing page. Is this template ever used in another instance?

Here is the output on a taxonomy term page:

<div class="term-listing-heading">
  <section id="taxonomy-term-1" class="taxonomy-term vocabulary-tags">
    <div class="content"></div>
  </section>
</div>
... followed by a list of nodes that match this term ...

It's just a bunch of empty tags! The heading isn't printed because it's a page, and <?php print render($content); ?> doesn't output anything.

Also, why are we using <section> for the wrapper? It doesn't seem to be wrapping anything besides the taxonomy term.

I have a feeling that I’m missing something obvious, like this template is used in another display where it lists multiple terms.

Jacine’s picture

Status: Needs review » Needs work
Issue tags: -D8H5 +sprint

Hmm... I'm pretty sure this is only used for taxonomy/term/x and taxonomy/term/x/feed, but yeah, it doesn't really seem ideal. The $content here is for the description of the taxonomy term. The term itself is the page title. If a description doesn't exist, then all that markup is pointless. When there are nodes that contain the tag, a list of teasers appear underneath (not inside this section). Maybe I'm missing something too, but it seems like this will never get a proper heading. There's also WAY too many wrappers here.

<div class="term-listing-heading">
  <section id="taxonomy-term-1" class="taxonomy-term vocabulary-tags">
    <div class="content">
        <div class="taxonomy-term-description">
          <p>The term description.</p>
       </div>
    </div>
  </section>
</div>

It seems like it wants to display a listing of terms per vocabulary, but I don't see anywhere this actually happens. Basically, I don't see a situation where this ever ends up being true (not even when terms are nested):

  <?php if (!$page): ?>
    <h2><a href="<?php print $term_url; ?>"><?php print $term_name; ?></a></h2>
  <?php endif; ?>

Does anyone know what the deal is here? If not, maybe we can ask @catch or @bangpound.

Jacine’s picture

Issue tags: -sprint

untagging.

sun’s picture

Component: theme system » taxonomy.module
Status: Needs work » Needs review
FileSize
2.25 KB

This template is the default output for taxonomy term entities, invoked via taxonomy_term_view().

Within Drupal core, this template is only used by taxonomy_term_page(), which is the taxonomy/term/% page.

On that page, it is only responsible for outputting the term's description and any other fields that may be attached to it - taxonomy terms are fieldable. $content may contain a plethora of custom fields, just like $content of a node can.

Since the taxonomy term description is not a field, but still a hard-coded property of the entity, taxonomy_term_view() only outputs the description when it is not empty. If you intend to touch this markup, note that there's some styling for it in taxonomy.css.

taxonomy_term_page() adds some wrapping HTML to the actual taxonomy term output, probably with the intention to delimit it from the other output on the page. The following nodes (if any) are not wrapped in any container, but appear directly after DIV.taxonomy-term-heading. If there are no nodes associated with the term, a plain paragraph appears instead, also not wrapped (and thus this para is not reliably selectable via CSS).

In total, this means that you currently get the following markup for the main page content:

DIV.taxonomy-term-heading
  DIV.taxonomy-term
    [H2 - only in teaser view]
    DIV.content
      [DIV.taxonomy-term-description - if any]
      [arbitrary attached fields - if any]

[nodes - if any]
[pager - if more than 10 nodes]
[P - if no nodes]

Outside of Drupal core, this template may be used by arbitrary functionality, just like any other entity. It is perfectly legit for a contributed or custom module to render a list of fully themed taxonomy terms in the teaser/summary view mode (or any other) and expect that to work. This is the case where the $page condition gets triggered and the $term_name should appear in the output.

With regard to semantics, the taxonomy term entity output itself is content that is identical to a node or comment. It has a heading/title and arbitrary field content.

I'd suggest to turn it into an ARTICLE (like any other entity) and remove the additional wrapping markup from the taxonomy term page output.

As far as I can see, none of the other entity theme templates are dealing with the special case of an entirely empty $content variable on the entity's own full page yet. Thus, if you have a node that only consists of a title (and no body), and you disable the authored/submitted output for the node type, then you equally get an ARTICLE container that is empty, which contains a DIV.content that is also empty. Therefore, this special case sounds like something that should be addressed in a separate issue/effort, and likely requires much more discussion and considerations.

For this plain HTML5 conversion issue, I'd recommend to just do what is necessary, and treat this template like any other entity theme template. Speaking of, the current template is totally missing all of the generic template variables, which means that other modules like Contextual links or RDF are not able to plug into it.

In total, I'd personally expect something like this, pretty much identical to the node page + comments markup/output:

ARTICLE.taxonomy-term
  [H2 - only in teaser view]
  DIV.content
    [DIV.taxonomy-term-description - if any]
    [arbitrary attached fields - if any]

SECTION.nodes
  [nodes - if any]
  [pager - if more than 10 nodes]
  [DIV.empty - if no nodes]
star-szr’s picture

Title: Convert taxonomy-term.tpl.php to HTML5 » Convert taxonomy-term.html.twig to HTML5
Status: Needs review » Needs work

This will need reworking now that taxonomy-term.tpl.php has been converted to Twig.

danielcalle’s picture

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

Status: Needs review » Needs work
Issue tags: +html5

The last submitted patch, drupal8.taxonomy-term-template.16.patch, failed testing.

danielcalle’s picture

Status: Needs work » Needs review
FileSize
2.07 KB

Reworking Sun's patch (#16) now that taxonomy-term.tpl.php has been converted to Twig.

PD: It's my first drupal patch, after the Seville Drupal Ladder by Seville Group.

The last submitted patch, drupal8.taxonomy_term_template-1190206-20.patch, failed testing.

mgifford’s picture

Issue summary: View changes
Issue tags: +Needs reroll
lokapujya’s picture

Issue tags: -Needs reroll
FileSize
1.89 KB

Reroll, setting to needs review for testbot.

lokapujya’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: 1190206-23.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

testing this patch without "full" display mode.

lokapujya’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update
Jeff Burnz’s picture

+++ b/core/modules/taxonomy/taxonomy.pages.inc
@@ -42,16 +42,20 @@ function taxonomy_term_page(Term $term) {
+  $build['nodes'] = array(
+    '#prefix' => '<section class="nodes">',
+    '#suffix' => '</section>',
+   );
...
       '#prefix' => '<p>',
       '#markup' => t('There is currently no content classified with this term.'),
       '#suffix' => '</p>',

Can we do these things in the template, rather than in a function - this leaves us with pretty hard coded markup thats rather difficult to change (for example the class of "nodes" is inadequate for my system requirements and I will need to change this), and use trans in the template for the message etc. Doable?

lokapujya’s picture

Jeff Burnz’s picture

lokapujya - I'll have to read up that issue, we might need to postpone or close this issue if this is getting replaced wholesale with a view.

mgifford’s picture

mgifford’s picture

Status: Needs work » Closed (duplicate)