Issue summary updated in comment #5.

Problem/Motivation

  1. in the documentation for field--taxonomy-term-reference.html.twig, some of the documentation has been improved:
    - * - label_hidden: Whether to show the field label or not.
    + * - label_hidden: Whether the field label has been set to hidden.
    - * - item_attributes: List of HTML attributes for each item.
    + * - item_attributes: HTML attributes for each item.
    - * @ingroup themeable
    
  2. In the code that outputs field items, the wrapper is missing the field-items class and any content attributes (e.g.: RDF):
    -  <div class="field-items"{{ content_attributes }}>
    +  <ul class="links">
    

Proposed resolution

  1. Make documentation consistent with field.html.twig.
  2. Make field item wrapper consistent with field.html.twig.

Remaining tasks

  1. Write a patch
  2. Review and RTBC
  3. Commit

User interface changes

  1. Field item wrapper is consistent with field.html.twig.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mparker17’s picture

Assigned: Unassigned » mparker17

Ok, I'll try to patch this.

mparker17’s picture

Assigned: mparker17 » Unassigned
FileSize
2.08 KB

Okay, here's a patch.

mparker17’s picture

Status: Active » Needs review

Let's see what Testbot thinks...

I am aware of two problems though:

  1. after the patch is applied, the label is a much smaller font-size than it was before, and,
  2. I forgot to update the docblock at the top of the twig file.
mparker17’s picture

FileSize
757 bytes
2.49 KB

after the patch is applied, the label is a much smaller font-size than it was before, and,

Whoops, this is not true — I'd reloaded the page while in the middle of writing the patch, so I'd seen the label without the field-label class on it.

I forgot to update the docblock at the top of the twig file.

Fixed in attached patch.

mparker17’s picture

Title: Bartik's taxonomy-term-reference template outputs label_attributes instead of title_attributes. » Bartik's taxonomy-term-reference template outputs label_attributes instead of title_attributes, diverges from core
Issue summary: View changes
Status: Needs review » Needs work

Comparing core/modules/system/templates/field.html.twig to core/themes/bartik/templates/field--taxonomy-term-reference.html.twig and comparing template_preprocess_field() to bartik_preprocess_field() reveals the display logic has diverged somewhat...

  1. In the documentation for field--taxonomy-term-reference.html.twig, some of the documentation has been improved:
    - * - label_hidden: Whether to show the field label or not.
    + * - label_hidden: Whether the field label has been set to hidden.
    - * - item_attributes: List of HTML attributes for each item.
    + * - item_attributes: HTML attributes for each item.
    - * @ingroup themeable
    
  2. In the code that outputs field items, the wrapper is missing the field-items class and any content attributes (e.g.: RDF):
    -  <div class="field-items"{{ content_attributes }}>
    +  <ul class="links">
    

... I think it's worth expanding the scope of this issue a bit to include those.

mparker17’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
28.63 KB

Here's a patch that includes the changes suggested in #5.

mparker17 queued 6: bartik_s-2298923-6.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: bartik_s-2298923-6.patch, failed testing.

mparker17’s picture

mparker17’s picture

Title: Bartik's taxonomy-term-reference template outputs label_attributes instead of title_attributes, diverges from core » Bartik's field--taxonomy-term-reference.html.twig doesn't output content_attributes, has out-of-date documentation

Fixing title too.

mparker17’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
677 bytes

Here's a patch.

The only difference is that I'm not changing label_attributes to title_attributes because that's done in #2301599: Regression: Bartik's field--taxonomy-term-reference.html.twig doesn't properly output label attributes.

star-szr’s picture

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

This needs a quick reroll because I changed label_attributes -> title_attributes in #2301599: Regression: Bartik's field--taxonomy-term-reference.html.twig doesn't properly output label attributes (oops!) :)

The last submitted patch, 11: bartik_s-2298923-11.patch, failed testing.

mparker17’s picture

I changed label_attributes -> title_attributes in #2301599 (oops!) :)

Actually, that's not an Oops because field.tpl.php uses title_attributes too — yay for new-found consistency between Bartik and Core! :D

mparker17’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.66 KB

Here you go...

mparker17’s picture

FileSize
1.6 KB

Whoops, fail, I attached the old patch I was supposed to re-roll.

The last submitted patch, 16: bartik_s-2298923-11.patch, failed testing.

star-szr’s picture

Status: Needs review » Needs work

Yeah I meant oops because I didn't know this issue existed :) Probably would have needed a small reroll anyway though.

Anyway, I was about to RTBC this, but:

+++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
@@ -20,7 +20,7 @@
-  <ul class="links">
+  <ul class="links field-items"{{ content_attributes }}>

This should be more along the lines of:

<ul class="links field-items {{ content_attributes.class }}"{{ content_attributes|without('class') }}>

Otherwise you could easily end up with two sets of class attributes. See also #2289511: [meta] Results of Drupalcon Austin's Consensus Banana and #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names. that are related to making that better.

(edited to add missing code tags)

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
766 bytes
1.32 KB

Incorporated #19 changes, please review updated patch.

trevjs’s picture

Reviewing

trevjs’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies cleanly, and seems consistent with field.html.twig

star-szr’s picture

Yup, that looks better. Thanks @er.pushpinderrana!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: bartik_s-2298923-20.patch, failed testing.

Status: Needs work » Needs review

Cottser queued 20: bartik_s-2298923-20.patch for re-testing.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fad8529 and pushed to 8.x. Thanks!

YesCT’s picture

Issue tags: -DX +DX (Developer Experience)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.