Simplify the HTML of field.html.twig [#3308309]

Problem/Motivation

The Drupal field template is quite verbose and can be hard to read. We start off checking for the label, then put in a conditional for if there are more than 1 field item (whether it's a multivalue field or not), then output certain divs depending on if the field has multiple values, then give an else version if not, and back to checking again about that label ... it gets tiresome.

Steps to reproduce

Open field.html.twig and see for yourself

Proposed resolution

Wouldn't it be great if we could simplify this to:

Be easier to read

Out the same markup, whether there is one item in my field or multiple (so we don't have different CSS/JS depending on how many values we output)
Have a conditional for styling differently if there is multiple (but this place as a modifier on the .field class, not as an additional .field__items class

Remaining tasks

Rewrite the field.html.twig template

User interface changes

None

API changes

None - my patch is against the starterkit_theme, but I'd like if we could (in D10) have this approach in our other themes too.

Data model changes

None

Release notes snippet

We have simplified the field.html.twig template. The only breaking change here is that we have a .field__items class wrapping the .field__item in all cases now, whether the field outputs multiple items or not. This is to make sure you can write the same CSS for single and multiple items. If you need to style something differently because it has multiple items, there is a .field--has-multiple-items modifier class at the .field level

This is based on a blog post I wrote called Simplified Drupal Field Template (but the same markup rendered)

Comment	File	Size	Author
#2	simplify-field-template-3308309-2.patch	1.52 KB	markconroy
#2	10.0.x: PHP 8.1 & MySQL 5.7 28,397 pass, 10 fail

Issue fork drupal-3308309

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3308309-simplify-html-of-field.html.twig changes, plain diff MR !7417
Check out this branch
3308309-simplify-the-html changes, plain diff MR !2786
Check out this branch
11.x compare
Check out this branch

1 hidden branch

9.5.x

changes, plain diff MR !7416

Check out this branch

About issue forks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

7 September 2022 at 07:58

markconroy created an issue. See original summary.

Comment #2

markconroy CreditAttribution: markconroy at Annertech for LocalGov Drupal commented 7 September 2022 at 08:11

Status:

Active

» Needs review

File	Size
simplify-field-template-3308309-2.patch	1.52 KB
10.0.x: PHP 8.1 & MySQL 5.7 28,397 pass, 10 fail

Adding patch.

Comment #3

markconroy CreditAttribution: markconroy at Annertech for LocalGov Drupal commented 7 September 2022 at 08:12

Issue summary:

View changes

Comment #4

7 September 2022 at 09:19

Status:

Needs review

» Needs work

The last submitted patch, 2: simplify-field-template-3308309-2.patch, failed testing. View results

Comment #5

markconroy CreditAttribution: markconroy at Annertech for LocalGov Drupal commented 7 September 2022 at 16:08

Status:

Needs work

» Needs review

Setting back to needs review for the tests to run again.

Comment #6

lauriii

he/him

Finnish

Finland

CreditAttribution: lauriii at Acquia commented 7 September 2022 at 16:12

FWIW, the test failures look legit and we should probably address those.

Comment #7

markconroy CreditAttribution: markconroy at Annertech for LocalGov Drupal commented 7 September 2022 at 18:04

@lauriii Can I presume those test failures are not because of my patch? I can't see any thing in it that would cause those failures, or is there something else you think I need to do to get the tests passing?

Comment #8

Chi CreditAttribution: Chi commented 8 September 2022 at 04:41

{% if multiple %}
  <div class="field__items">
{% endif %}

That's not specific to this issue but I'd like to indicate that IDEs have problems with syntax highlighting of such code.
Also having non-closed tags in the codition blocks makes it hurder to read and understantd even though it seems less verbose.

Comment #9

markconroy CreditAttribution: markconroy at Annertech for LocalGov Drupal commented 13 September 2022 at 09:30

I think the tests might be failing because our mark up for single items is changed now (with forcing the extra .field__items div to always be present), so the test for a single field item can’t check for .field > .field__item it needs to check for .field > .field__items > .field__item

Let's figure it out in Prague next week

Comment #10

sarahjean CreditAttribution: sarahjean at Digital Polygon commented 13 September 2022 at 14:41

I think this is great, I almost always wind up overriding the default field templates to pare back all the extra cases, not this exact way but similar idea.

Comment #11

wesruv CreditAttribution: wesruv commented 15 September 2022 at 04:01

Love this for starter kit, think it could be good for classy, but I wouldn't want this for lowest level field template & stable theme.

I'd prefer the default & stable templates prioritizing output over dev experience (without abandoning it).

Comment #12

23 September 2022 at 10:10

bserem made their first commit to this issue’s fork.

Comment #13

23 September 2022 at 10:31

bserem opened merge request !2786

Comment #14

tonypaulbarker CreditAttribution: tonypaulbarker at Annertech commented 23 September 2022 at 11:03

I think we are looking at

Line 52 of testEntityViewController (core/modules/system/tests/src/Functional/Entity/EntityViewControllerTest.php)

and I think we want to achieve something like

<div class="field field--name-name field--type-string field--label-hidden"><div class="field__items"><div class="field__item">' . $label . '</div></div></div>

But, we seem to have some other issues with the tests locally, so going to see how we go and send work in progress.

Comment #15

23 September 2022 at 12:02

kostask made their first commit to this issue’s fork.

Comment #16

bserem CreditAttribution: bserem at Annertech commented 23 September 2022 at 12:09

Status:

Needs review

» Needs work

Comment #17

tonypaulbarker CreditAttribution: tonypaulbarker at Annertech commented 23 September 2022 at 12:10

Status:

Needs work

» Needs review

-> Needs review for running tests

Comment #18

bserem CreditAttribution: bserem at Annertech commented 23 September 2022 at 12:17

1 file was hidden/shown/deleted

File	Size
simplify-field-template-3308309-2.patch	1.52 KB
10.0.x: PHP 8.1 & MySQL 5.7 28,397 pass, 10 fail

Comment #19

markconroy CreditAttribution: markconroy at Annertech for LocalGov Drupal commented 23 September 2022 at 12:34

Great work everyone. Thanks for moving this forward.

Comment #20

andy-blum

he/him/his

English

Ohio, USA

CreditAttribution: andy-blum at Lullabot commented 23 September 2022 at 13:47

Status:

Needs review

» Reviewed & tested by the community

The patched version of the twig is definitely easier to read, and there's no cost or risk associated with making this change in starterkits, but I fear it may be too late to get this change into D10.

The string returned from the test is has some ugly indentation, but it's a test and if it passes then I suppose ¯\_(ツ)_/¯

Pending successful tests, RTBC.

Comment #21

bserem CreditAttribution: bserem at Annertech commented 23 September 2022 at 19:21

Status:

Reviewed & tested by the community

» Needs work

Several other areas are affected by this, lots of tests to check (or change those tests to use stark)

Comment #22

tonypaulbarker CreditAttribution: tonypaulbarker at Annertech commented 24 September 2022 at 07:40

Some context to the tests.

They do not test anything in starterkit. They should be tested against Stark. They were changed to test against starterkit as a quick workaround when other themes were moved out of core. That is not really related to this issue (and no regression is caused) so it should not hold this up, we should open a new issue to tackle running against Stark.

We should only be concerned about coding standards of the indentation. If it passes coding standards, then I think this is good to go. Otherwise the indentation should be the only remaining thing to address.

This should ship with D10, otherwise we could start to run into backward compatibility issues.

Moving to review for check of coding standards of the indentation.

Comment #23

tonypaulbarker CreditAttribution: tonypaulbarker at Annertech commented 24 September 2022 at 07:42

Status:

Needs work

» Needs review

Comment #24

andy-blum

he/him/his

English

Ohio, USA

CreditAttribution: andy-blum at Lullabot commented 24 September 2022 at 21:05

Status:

Needs review

» Needs work

Tests are failing.

Comment #25

tonypaulbarker CreditAttribution: tonypaulbarker at Annertech commented 25 September 2022 at 16:15

Sorry @andy-blum you're quite right - we've more tests to work through.

Comment #26

25 September 2022 at 16:15

Version:

10.0.x-dev

» 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Comment #27

kostask CreditAttribution: kostask at Point Blank commented 10 April 2024 at 05:55

Issue tags:

+GreeceSpringSprint2024