Problem/Motivation

Follow-up to #2227601-51: [Follow-up] Add a "Visually-hidden" field label formatter.

  • In #2227601: [Follow-up] Add a "Visually-hidden" field label formatter, bartik_preprocess_field() was changed to use title_attributes instead of label_attributes, but we forgot to update field--taxonomy-term-reference.html.twig in that patch.
  • An extra array wrapper around a class name was introduced: $variables['title_attributes']['class'][] = array('field-label'); — this spits out Array instead of field-label

Proposed resolution

  1. Change field--taxonomy-term-reference.html.twig to output title_attributes.
  2. Remove the array wrapper around the 'field-label' class.

Remaining tasks

  1. Write patch
  2. Review + RTBC
  3. Commit

User interface changes

Label attributes on taxonomy-term-reference fields in Bartik will work.

API changes

None.

CommentFileSizeAuthor
#6 interdiff.txt757 bytesstar-szr
#6 2301599-6.patch1.7 KBstar-szr
#2 regression_bartik_s-2301599-2.patch1.69 KBmparker17

Comments

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Do we have automated tests for themes?

To manually test:

  1. Install D8 with this patch
  2. Go to node/add/article and add an article. Give it some taxonomy terms ("Tags").
  3. Go to admin/structure/types/manage/article/display and change the Label of the Tags field to Inline
  4. View the article you just created. The Tags label should be displayed inline with the classes. Inspect the classes on it. They should be field-label inline.
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Active » Needs review
StatusFileSize
new regression_bartik_s-2301599-2.patch1.69 KB

Patch attached.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Assigned: mparker17 » Unassigned
star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks great, thanks @mparker17! Regarding automated tests for themes the short answer is no :(. #2099579: Discuss automated visual regression testing is the first issue that comes to mind related to that. It sounds like in this case it would need to be paired with a "scenario" to reproduce the bug as well.

Markup before patch (including twig_debug information):

<!-- THEME DEBUG -->
<!-- CALL: _theme('field') -->
<!-- FILE NAME SUGGESTIONS:
   * field--node--field-tags--article.html.twig
   * field--node--field-tags.html.twig
   * field--node--article.html.twig
   * field--field-tags.html.twig
   x field--taxonomy-term-reference.html.twig
   * field.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/field--taxonomy-term-reference.html.twig' -->
<div class="field field-node--field-tags field-name-field-tags field-type-taxonomy-term-reference field-label-inline clearfix clearfix" data-quickedit-field-id="node/1/field_tags/en/full">
      <h3>Tags: </h3>
    <ul class="links">
          <li class="taxonomy-term-reference-0"><a href="/taxonomy/term/1" property="schema:about">one</a></li>
          <li class="taxonomy-term-reference-1"><a href="/taxonomy/term/2" property="schema:about">two</a></li>
          <li class="taxonomy-term-reference-2"><a href="/taxonomy/term/3" property="schema:about">three</a></li>
      </ul>
</div>

<!-- END OUTPUT from 'core/themes/bartik/templates/field--taxonomy-term-reference.html.twig' -->

Markup after patch (including twig_debug information):

<!-- THEME DEBUG -->
<!-- CALL: _theme('field') -->
<!-- FILE NAME SUGGESTIONS:
   * field--node--field-tags--article.html.twig
   * field--node--field-tags.html.twig
   * field--node--article.html.twig
   * field--field-tags.html.twig
   x field--taxonomy-term-reference.html.twig
   * field.html.twig
-->
<!-- BEGIN OUTPUT from 'core/themes/bartik/templates/field--taxonomy-term-reference.html.twig' -->
<div class="field field-node--field-tags field-name-field-tags field-type-taxonomy-term-reference field-label-inline clearfix clearfix" data-quickedit-field-id="node/1/field_tags/en/full">
      <h3 class="field-label inline">Tags: </h3>
    <ul class="links">
          <li class="taxonomy-term-reference-0"><a href="/taxonomy/term/1" property="schema:about">one</a></li>
          <li class="taxonomy-term-reference-1"><a href="/taxonomy/term/2" property="schema:about">two</a></li>
          <li class="taxonomy-term-reference-2"><a href="/taxonomy/term/3" property="schema:about">three</a></li>
      </ul>
</div>

<!-- END OUTPUT from 'core/themes/bartik/templates/field--taxonomy-term-reference.html.twig' -->
star-szr’s picture

star-szr
he/him
Primary language English
commented

And here's the key difference since I didn't realize that was going to look so blobby:

Before:
<h3>Tags: </h3>

After:
<h3 class="field-label inline">Tags: </h3>

star-szr’s picture

star-szr
he/him
Primary language English
commented
StatusFileSize
new 2301599-6.patch1.7 KB
new interdiff.txt757 bytes

Oh, one more thing to change outside the context of the patch, the docblock template variable name needs to be corrected.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

@Cottser awesome thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Nice catch. It's too bad we can't test for this. :(

Committed and pushed to 8.x. Thanks!

  • webchick committed 783db6d on 8.x 
    Issue #2301599 by Cottser, mparker17: Fixed Regression: Bartik's field--...

Status: Fixed » Closed (fixed)

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