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.
Issue summary updated in comment #5.
Problem/Motivation
- 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
- 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
- Make documentation consistent with
field.html.twig
. - Make field item wrapper consistent with
field.html.twig
.
Remaining tasks
- Write a patch
- Review and RTBC
- Commit
User interface changes
- Field item wrapper is consistent with
field.html.twig
.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#20 | bartik_s-2298923-20.patch | 1.32 KB | er.pushpinderrana |
#20 | interdiff-2298923-16-20.txt | 766 bytes | er.pushpinderrana |
#17 | bartik_s-2298923-16.patch | 1.6 KB | mparker17 |
Comments
Comment #1
mparker17Ok, I'll try to patch this.
Comment #2
mparker17Okay, here's a patch.
Comment #3
mparker17Let's see what Testbot thinks...
I am aware of two problems though:
Comment #4
mparker17Whoops, 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.Fixed in attached patch.
Comment #5
mparker17Comparing
core/modules/system/templates/field.html.twig
tocore/themes/bartik/templates/field--taxonomy-term-reference.html.twig
and comparingtemplate_preprocess_field()
tobartik_preprocess_field()
reveals the display logic has diverged somewhat...field--taxonomy-term-reference.html.twig
, some of the documentation has been improved:field-items
class and any content attributes (e.g.: RDF):... I think it's worth expanding the scope of this issue a bit to include those.
Comment #6
mparker17Here's a patch that includes the changes suggested in #5.
Comment #9
mparker17#2227601: [Follow-up] Add a "Visually-hidden" field label formatter did part of the original scope of this issue. #2301599: Regression: Bartik's field--taxonomy-term-reference.html.twig doesn't properly output label attributes should fix the twig file.
Updating the scope of this issue accordingly.
Comment #10
mparker17Fixing title too.
Comment #11
mparker17Here'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.
Comment #12
star-szrThis 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!) :)
Comment #15
mparker17Actually, that's not an Oops because
field.tpl.php
usestitle_attributes
too — yay for new-found consistency between Bartik and Core! :DComment #16
mparker17Here you go...
Comment #17
mparker17Whoops, fail, I attached the old patch I was supposed to re-roll.
Comment #19
star-szrYeah 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:
This should be more along the lines of:
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)
Comment #20
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedIncorporated #19 changes, please review updated patch.
Comment #21
trevjs CreditAttribution: trevjs commentedReviewing
Comment #22
trevjs CreditAttribution: trevjs commentedPatch applies cleanly, and seems consistent with field.html.twig
Comment #23
star-szrYup, that looks better. Thanks @er.pushpinderrana!
Comment #26
star-szrComment #27
alexpottCommitted fad8529 and pushed to 8.x. Thanks!
Comment #28
YesCT CreditAttribution: YesCT commented