Problem/Motivation

The markup that is produced for a field is having a serious case of divitis - Looking at the discussions and the principles we build the frontend on for twig, that is wrong(tm)

Single value fields
A single field gets wrapped into 3 divs that is making the field fugly to work with out of the box, we should follow the waypoints of theming in drupal8 that is start with a minimum then build on top of that if a theme needs it.

Proposed resolution

A field can be rendeded in 4 differnt variations:
single field
single field with label
multiple field
multiple field with label

For a single field we will combine it all into a one div wrapper:

<div class="field field--[modifiers] field__item">
 foo
</div>

With label we add a wrapper cause it make sense

<div class="field field--[modifiers] field--label-[mod]">
  <div class="field__label">label</div>
  <div class="field__item field__item--[modifiers]">foo</div>
</div>

For the multiple fields with label we add in all the divs:

<div class="field field--[modifiers] field--label-[mod]">
  <div class="field__label">label</div>
  <div class="field__items">
    <div class="field__item field__item--[modifiers]">foo</div>
    <div class="field__item field__item--[modifiers]">bar</div>    
  </div>  
</div>

and for a multiple value field without a label we remove the unnecessary divs for wrappers

<div class="field field--[modifiers] field__items">
  <div class="field__item field__item--[modifiers]">foo</div>
  <div class="field__item field__item--[modifiers]">bar</div>    
</div>

field.twig template:

{%
  set classes = [
    'field',
    'field--type-' ~ field_type|clean_class,
    'field--name-' ~ field_name|clean_class,
    'field--label-' ~ label_display,
  ]
%}
{#
The field template is divided into 4 different variations all depending
on how the field is configured.
We create 4 variations of the markup to prevent divits & make it easier to
modify later on
* Label & multiple fields
*
#}
{% if not label_hidden %}
{# The field have a label. #}
  {% if multiple %}
    {# Multiple items field with a label #}
    <div{{ attributes.addClass(classes) }}>
      <div{{ title_attributes.addClass('field__label') }}>{{ label }}:</div>
      <div{{ content_attributes.addClass('field__items') }}>
      {% for item in items %}
        <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
      {% endfor %}
      </div>
    </div>

  {% else %}
    {# Single item field with a label #}
    <div{{ attributes.addClass(classes) }}>
      <div{{ title_attributes.addClass('field__label') }}>{{ label }}:</div>
      {% for item in items %}
        <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
      {% endfor %}
    <div>
  {% endif %}

{% else %}
{# The field have no label. #}
  {% if multiple %}
    {# Multiple items field with no label #}
    <div{{ attributes.addClass(classes).addClass('field__items') }}>
      {% for item in items %}
      <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
      {% endfor %}
    </div>

  {% else %}
    {# single item field with no label #}
    {% for item in items %}
      <div{{ item.attributes.addClass(classes).addClass('field__item') }}>{{ item.content }}</div>
    {% endfor %}
  {% endif %}

{% endif %}

Remaining tasks

wait for theme_field to be removed #1987398: field.module - Convert theme_ functions to Twig
wait for theme_field__node__title #2231505: Convert theme_field__node__title() to Twig

User interface changes

none

API changes

Files: 
CommentFileSizeAuthor
#366 Screen Shot 2015-08-27 at 12.21.32 pm.png378.21 KBdman
#353 interdiff.txt1.29 KBscor
#353 field_default_markup-2214241-353.patch42.35 KBscor
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,974 pass(es). View
#352 interdiff.txt416 byteslauriii
#352 field_default_markup-2214241-352.patch42.48 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,938 pass(es). View
#349 field_default_markup-2214241-349-failing.patch42.24 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,878 pass(es), 3 fail(s), and 314 exception(s). View
#336 2214241-336-field-default-markup.patch41.9 KBmdrummond
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,176 pass(es). View

Comments

mortendk’s picture

longwave’s picture

Should multivalue fields be <li>s inside a <ul> container? They are a list of items, after all.

Sam152’s picture

I don't know if <label> is the correct tag to use in this situation. I think semantically they're probably more suited to form elements. In the HTML5 spec, labels (4.10.4) are specifically a subsection of forms (4.10), however there doesn't seem to be any explicit prohibition on their use outside forms. I would however argue that a strict interpretation of the spec, would indicate the above use incorrect.

dcmouyard’s picture

The <label> element should only be used to reference labelable elements. You can create custom labelable elements via JS, but the default elements are:

  • button
  • input
  • keygen
  • meter
  • output
  • progress
  • select
  • textarea
mortendk’s picture

Issue summary: View changes

updated the issue fixed the label to a more correct h3

mortendk’s picture

Issue summary: View changes

added a twig template example

baronmunchowsen’s picture

My preference would be for a div to wrap the multiple value fields, rather than a section tag. From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section

Do not use the element as a generic container; this is what

is for, especially when the sectioning is only for styling purposes. A rule of thumb is that a section should logically appear in the outline of a document.

Specifically:

A rule of thumb is that a section should logically appear in the outline of a document.

barraponto’s picture

I was about to write precisely what @baronmunchowsen wrote, I +1 that :)

mortendk’s picture

check that would be to much to have the section there as a defuault :)

mortendk’s picture

Issue summary: View changes

i have modified the proposed template, so no section or h3

field.twig template:

<div class="soitseasy-to-add-new {{ attributes.class }}" {{ attributes }}>
  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}</div>
  {% endif %}
  
  {% for delta, item in items %}

    {% if loop.length > 1 %} 
    <ul class="field-items" {{ content_attributes }}>
      <li class="field-item">{{ item }}</li>
    </ul>
    {% elseif  %}  
      {# print only the data #}
      {{ item }}
    {% endif  %}  

  {% endfor %}
</div>
mortendk’s picture

Issue summary: View changes
trevortwining’s picture

As far as the semantics go, I love this SO much better than what's in D7.

I agree with the comments around use of the label tag. The solution in #10 is semantic and general enough for out of the box use. In some cases I would override the label in this template and use heading tags, but that's not often enough that we should try to account for it here.

I haven't been following a lot of the work yet and hoping to get more involved soon, but if this what's going into D8 so far, I'm very happy indeed.

mdrummond’s picture

On one hand I like the simplicity here. I'm a bit concerned that the markup pattern is going to vary based on how many items a field has.

For example let's say I have a node reference field to show related pages. If I reference two pages, I get a list. If there's just one, no list.

If I want to style the individual page reference, I could use .field-item when there is a list. But if there is only one item, then I need to target the parent div. But if I do that, then it may mess up the styles when there is a list.

I'm beginning to understand why all the nested divs now.

mdrummond’s picture

Maybe wrapping item in span class=field-item would help.

geekyMoa’s picture

I think this is all about providing sensible defaults, and making them super-easy to change. Seems like #10 does exactly that. A list for a list of field items in a multiple value field makes much more sense than a bunch of divs, but considering all the different ways a field can be used, using a section and/or a heading tag has the potential of breaking document outlines in all sorts of interesting ways.

mdrummond, valid point. My gut reaction would be to add the field-item class to the wrapper div in case of there being only one item in a list, but if there are more items, add it to individual list items. Feel free to poke holes in that idea.

mdrummond’s picture

+1 to field-item on wrapper for one item

mdrummond’s picture

That would require CSS to have two selectors:

.field-name .field-item,
.field-name.field-item {
}

Could we have a .field-name-item class?

mortendk’s picture

about the classes we can have them all : #2217731: Move field classes out of preprocess and into templates

For example let's say I have a node reference field to show related pages. If I reference two pages, I get a list. If there's just one, no list.

well the fields are all a bit different so what i would have in that example would probably be a nav element instead

<nav class=" {{ attibutes.class }}"  {{ attibutes }}>
<ul class="field-items" {{ content_attributes }}>
      <li class="field-item">{{ item }}</li>
    </ul>
</nav>

My point here is that each field is gonna be "special" in some way or the other and its impossible to make something that will work for everything all the time.
By having a clean and easy to add to template makes it easy for the themer to fix each field the way the want to, so if you like the div div div ivitis you can add that in ;)

mdrummond’s picture

Status: Active » Needs review

Here's how we can solve this.

In field.module, within template_preprocess_field, we add this:

$variables['multiple'] = $element['#cardinality_multiple'];

That checks to see if a field is capable of having multiple field items or not.

Then we can change the template like so:

<div class="{% if multiple == false %}field-item{% endif %}{{ attributes.class }}" {{ attributes }}>
  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}</div>
  {% endif %}
  {% if multiple %}
    {% for delta, item in items %}
      <ul class="field-items" {{ content_attributes }}>
        <li class="field-item">{{ item }}</li>
      </ul>
    {% endfor %}
  {% else %}
    {# print only the data #}
    {{ item }}
  {% endif %}  
</div>

With that, if a field can have multiple items, it's in a list, regardless of how many items are in that field. That's nice and consistent.

If you want to use CSS to style every .field-item, regardless of which field it is a part of, you can do so. If you want to target a .field-item for a particular field, you should know if it allows multiple values or not, and then use .field-name .field-item or .field-name.field-item as necessary.

Seems fairly clean and simple. Thoughts?

barraponto’s picture

I find @mdrummond solution acceptable.

derheap’s picture

First of all:
The <ul> is only necesary once: (Should be outside the loop:)

<div class="{% if multiple == false %}field-item{% endif %}{{ attributes.class }}" {{ attributes }}>
  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}</div>
  {% endif %}
  {% if multiple %}
      <ul class="field-items" {{ content_attributes }}>´
    {% for delta, item in items %}
        <li class="field-item">{{ item }}</li>
    {% endfor %}
      </ul>
  {% else %}
    {# print only the data #}
    {{ item }}
  {% endif %}
</div>

And why do we need the outer <div>?
I always delete it … never need it.

mdrummond’s picture

Oops, sorry on putting the ul tags inside the for loop.

If the field allows multiple items, and it's outputting a list, and there's no label, then yes, any field classes could probably go on the ul.

If there is a label, I could see there being a use for having a wrapper around the label and the list, so that you can target the bundle.

If it is a single item, with no list involved, I'd think in most cases you'd want some sort of wrapper around the field data. Otherwise it's just going to clump up next to whatever is before or after it.

In most cases, if there's a single element, I'd probably have something like:

<p>{{ item }}</p>

If there was a label, I'd probably have:

<p><strong>{{ label }}:</strong> {{ item }}</p>

But this is supposed to be flexible, so I'm fine if we're using divs instead of p and strong.

I'd really only want that mess of classes unless I had a real reason to target those individual fields, otherwise I'd prefer removing all of them.

If I can do that easily with template files, ok.

jjcarrion’s picture

Assigned: Unassigned » jjcarrion
FileSize
18.7 KB
71.31 KB
2.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 713462-field-default-markup-removing-the-divitis-23.patch. Unable to apply patch. See the log in the details link for more information. View

I've write the patch following the idea of mdrummond, I've added a variable for multiples fileds to use it later in twig template.

I've tested it for text and image values (I've attached two screenshots) and works good.

jjcarrion’s picture

FileSize
1.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,785 pass(es), 27 fail(s), and 0 exception(s). View

reroll because the template_preprocess_field function has been moved to /core/includes/theme.inc

rteijeiro’s picture

Status: Needs work » Needs review

Let's make testbot work a little :P

rteijeiro’s picture

Do we really need &nbsp; here? Can we get rid of it? Ideas?

+++ b/core/modules/system/templates/field.html.twig
@@ -31,11 +31,16 @@
+    <h3 class="field-label"{{ title_attributes }}>{{ label }}:&nbsp;</h3>
dcmouyard’s picture

I’ve been trying to follow the Drupal 8 CSS architecture guidelines for the past 8 months or so and have ended up having "field" be a SMACSS component. Thus my markup looks like this:

<div class="field field--name/type/etc">
  <div class="field__label"></div>
  <ul class="field__items">
    <li class="field__item"></li>
    <li class="field__item"></li>
  </ul>
</div>

I’m not a fan of using <h3> for the label since it’s highly dependent on the heading structure of the page.

I really like the idea of removing the items and item wrappers for non-multivalue fields. Also, since we’re using <ul> for the field items, we could probably get away with removing the field item class.

jjcarrion’s picture

Assigned: jjcarrion » Unassigned
mortendk’s picture

+++ b/core/modules/system/templates/field.html.twig
@@ -31,11 +31,16 @@
+    <h3 class="field-label"{{ title_attributes }}>{{ label }}:&nbsp;</h3>

we should keep this as a div its more generic & we dont wanna start a bikeshed about a h3 - we can always do a follow up issue + remove "&nsp;" :)

jjcarrion’s picture

Assigned: Unassigned » jjcarrion
jjcarrion’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,714 pass(es), 24 fail(s), and 0 exception(s). View
505 bytes

Here is the new patch and the interdiff changing the h3 to a div.

galooph’s picture

Assigned: jjcarrion » galooph

Taking a look at fixing the failing tests.

galooph’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,717 pass(es), 21 fail(s), and 0 exception(s). View

Right, the tests are ok, I think - the fails were down to an extra space before the </li> in the patch:

-      <div class="field-item"{{ item_attributes[delta] }}>{{ item }}</div>
+        <li class="field-item"{{ item_attributes[delta] }}>{{ item }} </li>

The tests look for text enclosed between > and <, eg $this->assertRaw('>' . $newterm1 . '<', 'First new term displayed.');

I've removed the extra space and run some of the failing tests locally and they pass ok.

Lets see what the testbot thinks.

galooph’s picture

Status: Needs review » Needs work

Some of the other tests still fail locally, so I'll investigate further.

galooph’s picture

Fixed the failing Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() tests by changing the xpath to match the new markup.

-      $elements = $this->xpath('//div[text()="' . $output . '"]');
+      $elements = $this->xpath('//div/div/div/following-sibling::text()[contains(.,"' . $output . '")]');

Moving onto the next failing tests.

galooph’s picture

Started to look at the failing RDF tests and the patch actually removes some of the RDF markup. Looking at the Drupal\rdf\Tests\CommentAttributesTest->_testBasicCommentRdfaMarkup() test, the original markup was:

<div class="content">
  <span rel="sioc:reply_of" resource="/node/1" class="rdf-meta"></span>
  <div class="field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden">
    <div class="field-items">
      <div class="field-item" property="content:encoded"><p>vT3AygB4</p>
      </div>
    </div>
  </div>
</div>

and the markup after the patch is:

<div class="content">
  <span rel="sioc:reply_of" resource="/node/1" class="rdf-meta"></span>
  <div class="field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden">
    <p>o76HBbbM</p>
  </div>
</div>

so we lose the property="content:encoded", causing the test to fail.

galooph’s picture

With the patched field.html.twig template, we're losing the content_attributes and item_attributes information when a field doesn't have multiple set.

  {% if multiple %}
      <ul class="field-items" {{ content_attributes }}>
    {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
    {% endfor %}
      </ul>
  {% else %}
    {{ items }}
  {% endif %}

This is causing some of the RDF tests to fail, see #40 above. How can we include the attributes for non-multiple fields without bloating the markup? If someone can point me in the right direction, I'm happy to rework the tests to suit.

galooph’s picture

Assigned: galooph » Unassigned
galooph’s picture

Updated the patch of the work so far from #40 to tidy up the xpath statement with a placeholder.

-      $elements = $this->xpath('//div[text()="' . $output . '"]');
+      $elements = $this->xpath('//div/div/div/following-sibling::text()[contains(.,:output)]', array(':output' => $output));
galooph’s picture

FileSize
2.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,793 pass(es), 2 fail(s), and 0 exception(s). View

After discussing this with @ jjcarrion, I tried reintroducing the div around fields that don't have multiple set.

The field.html.twig template now looks like

  {% if multiple %}
      <ul class="field-items" {{ content_attributes }}>
    {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
    {% endfor %}
      </ul>
  {% else %}
    {% for delta, item in items %}
        <div class="field-item"{{ content_attributes }}{{ item_attributes[delta] }}>{{ item }}</div>
    {% endfor %}
  {% endif %}

and with this change, all the failed tests pass ok locally.

Running the patch through the testbot to see if it works.

longwave’s picture

Status: Needs work » Needs review
mdrummond’s picture

I wonder if it's simpler to just do this:

{% if multiple %}
  <div class="{{ attributes.class }}"{{ attributes }}>
     {% if not label_hidden %}
       <div class="field-label"{{ title_attributes }}>{{ label }}</div>
     {% endif %}
     <ul class="field-items" {{ content_attributes }}>
     {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
      {% endfor %}
      </ul>
  </div>
  {% else %}
  {% for delta, item in items %}
    <div class="field-item{{ attributes.class }}"{{ attributes }}{{ content_attributes }}{{ item_attributes[delta] }}>
     {% if not label_hidden %}
       <div class="field-label"{{ title_attributes }}>{{ label }}</div>
     {% endif %}
     {{ item }}
    </div>
  {% endfor %}
{% endif %}

An alternative would be:

  <div class="{% if multiple == false %}field-item{% endif %}{{ attributes.class }}"{{ attributes }}{% if multiple == false %}{{ content_attributes }}{% for delta, item in items %}{{ item_attributes[delta] }}{% endfor %}{% endif %}>
    {% if not label_hidden %}
      <div class="field-label"{{ title_attributes }}>{{ label }}</div>
    {% endif %}
    {% if multiple %}
    <ul class="field-items" {{ content_attributes }}>
      {% for delta, item in items %}
        <li class="field-item"{{ item_attributes[delta] }}>{{ item }}</li>
      {% endfor %}
    </ul>
    {% else %}
      {{ item }}
    {% endif %}
  </div>

The latter has two additional if multiple statements, but is a bit more DRY, as the field-label if block only appears once.

This would allow us to have just one div if there's only one item... and a whole mess of classes and attributes on the wrapper div.

mortendk’s picture

looks good in #47 i would go with the first option its easier to read & understnad

galooph’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,788 pass(es), 19 fail(s), and 0 exception(s). View

Doh, the Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() tests failed now that the markup had changed again. Still, 2 fails is better than 24 :-)

Here's the patch without the OptionsFieldUITest xpath change together with the first option from #47.

mortendk’s picture

Status: Needs review » Needs work
Related issues: +#2227395: field items class follow the drupal coding standards
scor’s picture

@galooph the RDF tests should pass as long as you keep "item_attributes[delta]" in the HTML element wrapping the field value, whether it's a multiple value or single value. The actual HTML element doesn't matter for RDFa, it can be div, span, ul or li, the only thing that matters in RDFa is the attributes.

galooph’s picture

Thanks @scor, that's what we figured. The changes suggested in #47 add the item_attributes back in.

aboros’s picture

Assigned: Unassigned » aboros
galooph’s picture

Checking out the test fails from the patch in #49.

The Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() test can be fixed with the xpath tweak, as before.

The Drupal\rdf\Tests\CommentAttributesTest->_testBasicCommentRdfaMarkup() test seems to fail due to some extra whitespace

The test sets up an expected value array:

    // Comment body.
    $expected_value = array(
      'type' => 'literal',
      'value' => $comment->comment_body->value . "\n",
      'lang' => 'en',
    );

but checking the output of debug($graph) in the test, we're actually getting:

        array (
          'type' => 'literal',
          'value' => '
          cB74xflH

    ',
          'lang' => 'en',
        ),
aboros’s picture

FileSize
108.21 KB
108.21 KB

Applying #49 results in double printed field label, i assume this is not meant to work like this:

Which version should stay? I am not a big fan of having ": " after every field label by default.

aboros’s picture

Assigned: aboros » Unassigned
FileSize
1.14 KB
2.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,921 pass(es), 17 fail(s), and 0 exception(s). View

The change in #49 fixes the Drupal\options\Tests\OptionsFieldUITest->testNodeDisplay() test so i added it. I also found that the field label was printed twice in field.html.twig, fixed that. Unfortunately it still fails some RDFa and other tests even after applying the final patch from https://drupal.org/node/2226233. Still needs work but i am not sure i can fix the tests, making it unassigned.

joelpittet’s picture

Some notes to have a look at from #7. Thanks for the patch @aboros!

  1. +++ b/core/includes/theme.inc
    @@ -2418,6 +2418,12 @@ function template_preprocess_field(&$variables, $hook) {
    +  // We have to know if the field has multiple values to create a simple value
    +  // or a list.
    +  if (count($variables['items']) > 1 ) {
    +    $variables['multiple'] = TRUE;
    +  }
    +
    
    +++ b/core/modules/system/templates/field.html.twig
    @@ -30,12 +30,27 @@
    +{% if multiple %}
    

    This can be done in the twig template with items.length > 1.

  2. +++ b/core/modules/system/templates/field.html.twig
    @@ -30,12 +30,27 @@
    +     <ul class="field-items" {{ content_attributes }}>
    

    Remove the space before the print tags {{}}. Each attribute prints a space before it automagically.

  3. +++ b/core/modules/system/templates/field.html.twig
    @@ -30,12 +30,27 @@
    +  {% else %}
    

    This else should be pulled back a tab.

  4. +++ b/core/modules/system/templates/field.html.twig
    @@ -30,12 +30,27 @@
    +    <div class="field-item{{ attributes.class }}"{{ attributes }}{{ content_attributes }}{{ item_attributes[delta] }}>
    

    This is asking for duplicate attributes, you'd likely want to merge them before hand in the preprocess onto something like item.attributes and then just have item.content/item.value? That's just my preference because I'd rather not use [delta]

galooph’s picture

Assigned: Unassigned » galooph

Picking this back up to investigate the failing RDF tests.

galooph’s picture

I've finally got my head around the EasyRdf library that's used in the RDF tests and I've got the 'RDFa markup of comments' test group to pass now.

I'll carry on with the next failing RDF test and post a patch later on.

galooph’s picture

Status: Needs work » Needs review
FileSize
2.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,826 pass(es). View

After lots of advice from @scor, we realised that the RDF tests that were failing were down to the new markup structure. @mdrummond came up with a revised twig template that maintained the RDF validation.

I've run all the failing tests locally and they all pass. The only test I've modified in the end is the OptionsFieldUITest, and then only to use a placeholder.

Changing the status back to Needs Review to trigger the testbot.

scor’s picture

And the tests are passing! The only requirement for RDF is to have the {{ item_attributes[delta] }} in the direct wrapping HTML element of the field item output, so yeah, this works:

<whatever ...{{ item_attributes[delta] }}>{{ item }}</whatever>
joelpittet’s picture

Thanks for fixing #58.2 @galooph could you have a look at addressing the other issues brought up in #58?

Also mind posting an interdiff with the changes? Here's a git workflow I use that may help you: http://pittet.ca/drupal/sprint/patch

Otherwise the official docs are at https://drupal.org/documentation/git/interdiff

joelpittet’s picture

An example of what I'm talking about in #58.4
@see #1939062-120: Convert theme_item_list() to Twig

scor’s picture

Let me raise a potential pitfalls for themers with the markup outlined in the OP. Imagine you have set a field allowing multiple values. You create a new node so you can start theming. You create 3 values for that field so you have a good scenario to theme against. Your CSS could look like this:

field-items field-item.li {color: red}

It will work, until a user submits a node with only one value for that field, in which case the CSS path will be different: field-items will be missing, and there will be no li. One could argue that the CSS could be designed for both use cases, but come on, that's no intuitive and people will forget. Maybe if the logic to switch was based on the field settings, and not the actual number of items being rendered, this problem would go away.

joelpittet’s picture

Completely agree with @scor. Field settings would be a better way to determine the element changes but with element changes on |length would be unintuitive. Though for anybody who would like to do that in their own site, it would be super trivial, by overwriting this template with {% if items|length > 1 %} ul list {% else %} div {% endif %}

mdrummond’s picture

I think there might be some potential issues with this structure, but I don't think that's a particularly good example.

If we're going to go through all of the effort to have a billion classes in pursuit of SMACSS nirvana, then it's not unreasonable to expect people to use those classes.

If what you want to do is to make each field item red, great, do this:

.field-item {
  color: red;
}

If you want each field item to be red whether it is in a list or not, then great, don't make your selector dependent on the existence of .field-items.

If you want to handle the padding around the list as a whole, then using the .field-items is a good and necessary choice.

It's possible the .field-label could be an issue, as it is outside .field-items when there are multiple field items, but inside .field-item if there is just one.

But again, if you want to change the styles of .field-label, then just target that class as a selector. And if you don't want .field-item to pollute the styles of .field-label, then don't use generic descendant selectors with .field-item.

Sorry to get all ranty, but I've never been thrilled with the divitis and classitis inherent in Drupal. That said, classitis is a potential cure for divitis if we take advantage of OOCSS principles. That at least lets us kill a good amount of the divitis. And then if people want to kill off the abundance of classes, particularly if they're developing their CSS with Sass, then more power to them.

mortendk’s picture

as a guideline from the theming community - this have been discussed alot over the last 2 years -> https://drupal.org/node/2008464#principles
this falls under the 80% - its not the goal of the theme system to create markup that takes care of 100% usecases - anyways im gonna document this now, so we can move forward :)

mortendk’s picture

To make sure were not doing anything blindly i have create a test page with the different changes that the patch in #61 is doing
http://mortendk.github.io/drupal8-fields

Theres some issues with that empty div that comes out with no class inside of it, wonder why that is there.
But besides of that i do think that were doing the 80%

galooph’s picture

FileSize
2.33 KB
2.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,808 pass(es). View

Here's a revised patch addressing #58.1 and #58.3

I'm not too sure about #58.4, @joelpittet, but happy to take a look if you can give me a few more pointers.

dman’s picture

I'm really concerned about the problem described in #65.

The idea that the actual structure of the markup could be one way today, and another way tomorrow depending on whether there are one or two values available is a problem. It's going to cause shocks and swearing at the system - by the themers - during testing.

The themers I work with look at the markup being produced, figure out a pattern and then build CSS to match it.
They can built a perfectly working result based on perfectly working site markup. This patch makes it possible and likely that that theme will blow up in the clients face as soon as they enter only one value, because all the mockups had more than one.

During the birth of CCK, at a technical level, all our node fields were sometimes singles, sometimes multiples. The attributes attached to the $node were sometimes strings, and sometimes arrays. You could never be sure what to expect, and all field handler code was layered with is_array() checks to figure out what to do with the input.
And then this was fixed. Multiple field values became supported properly as the only way to do it, each single value became an array of 1, but always an array, and vast amounts of logic were simplified.
Yes, it's a little annoying to have to go $node->fieldname[0]['value'] instead of just $node->fieldname['value'] but it's 100% consistent and doesn't switch around based on what the content is doing today.
There is now much less if-then-but work done in managing field rendering.

What this patch does is bring back the inconsistency and unpredictability that was seen in the early-cck days, and places new if-then work into the CSS.

I predict pain. Pain at the themers end, as this makes their life less predictable.

( sorry I'm late, and I see this was already brought up earlier. I only noticed this when RDF was brought up (good work there!). I'm just thinking of the new training we have to do down the road, and the landmines this lays. )

joelpittet’s picture

  1. +++ b/core/modules/system/templates/field.html.twig
    @@ -43,12 +43,12 @@
    +      <div class="field-item{{ attributes.class }}"{{ attributes }}>
    
    +++ b/core/modules/system/templates/field.html.twig
    @@ -43,12 +43,12 @@
    +      <div class="field-item{{ attributes.class }}"{{ attributes }}{{ content_attributes }}{{ item_attributes[delta] }}>{{ item }}</div>
    

    Ok so here's the thing, the attribute's "values" aren't smart like the name/value attribute pairs when it comes to putting spaces in-front. So you don't want to do this for the class pull outs. Though if you'd like to avoid the possibility of a space in the attribute at the end you can do this trick {{ attributes.class|merge(['field-items']) }}

I'm still on the side of scor's point in #65 and echo what @dman said in #71.
Though nice work on the demo page @mortendk: http://mortendk.github.io/drupal8-fields/

Re: #70 this line in the preprocess:

  foreach ($variables['items'] as $delta => $item) {
    $variables['item_attributes'][$delta] = !empty($element['#items'][$delta]->_attributes) ? new Attribute($element['#items'][$delta]->_attributes) : clone($default_attributes);
  }

Can become:

  foreach ($variables['items'] as $delta => $item) {
    $variables['items'][$delta]['value'] = $item;
    $variables['items'][$delta]['attributes'] = !empty($element['#items'][$delta]->_attributes) ? new Attribute($element['#items'][$delta]->_attributes) : clone($default_attributes);
  }

Then you can remove the delta and array access in Twig.

      {% for item in items %}
        <li class="field-item {{ item.attributes.class }}"{{ item.attributes }}>{{ item.value }}</li>
      {% endfor %}
dman’s picture

I could deal with switching markup if it was business rule - field cardinality is 1.
But that's not what is being checked here - it's switching due to there being just one of possibly many today.

galooph’s picture

Thanks for the help, @joelpittet - I think I've got the changes from #72 applied correctly now, but please double check!

I tried the twig merge you mentioned, but that threw an error - "Twig_Error_Runtime: The merge filter only works with arrays or hashes in "core/themes/bartik/templates/node.html.twig" at line 92. in twig_array_merge() (line 673 of core/vendor/twig/twig/lib/Twig/Extension/Core.php)."

mdrummond’s picture

Consistency is good. It just gets difficult to balance that with markup that ends up looking somewhat ridiculous, purely for the sake of consistency.

We do have a few different scenarios that are possible here.

We could have multiple field items, and a label for those field items. In that case, it's entirely logical that we'd have a structure that looks like this:

<div class="field-wrapper {{ attributes.class }}"{{ attributes }}>
  <div class="field-label {{ title_attributes.class }}"{{ title_attributes }}>{{ label }}:&nbsp;</div>
  <ul class="field-items {{ content_attributes.class }}" {{ content_attributes }}>
  {% for delta, item in items %}
    <li class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
  {% endfor %}
  </ul>
</div>

That's a very sensible HTML structure. There are a couple divs, but they make sense.

If there's just one item in a field, and there is a label, then it feels silly to have field-item wrapped in field-items.

There is a difference, however, between there being one item in a field that always has one item in a field, and one item in a field that sometimes has one item in a field.

I think it's a point well taken that maybe what we should be testing is if the field is capable of having multiple items rather than that plus if a field actually has multiple items. While it's a bit of extra HTML, it would be more consistent to keep the ul.field-items for fields that can have multiple items, even when there is only one.

The other scenario is a field that only ever has one item. Sensible markup for that would be:

{% for delta, item in items %}
<div class="field-wrapper {{ attributes.class }} {{ content_attributes.class }}"{{ attributes }}{{ content_attributes }}>
  <div class="field-label {{ title_attributes.class }}"{{ title_attributes }}>{{ label }}:&nbsp;</div>
  <div class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
</div>
{% endfor %}

If it's possible, it might be nice to merge the attributes and content_attributes in pre-process.

Anyhow, putting a field-wrapper around field-item makes sense here. What doesn't make sense would be to include field-items around field-item, particularly if this is only for fields that can only ever have one item.

The last scenario is that we have a field item, but it doesn't have a label. That's where it gets tricky. The consistent thing to do would be:

{% for delta, item in items %}
<div class="field-wrapper {{ attributes.class }} {{ content_attributes.class }}"{{ attributes }}{{ content_attributes }}>
  <div class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
</div>
{% endfor %}

What feels frustrating here is that the field-wrapper seems extraneous and needless cruft. It's consistent cruft, but cruft all the same. You'd get something similar for field-items with no label.

<div class="field-wrapper {{ attributes.class }}"{{ attributes }}>
  <ul class="field-items {{ content_attributes.class }}"{{ content_attributes }}>
  {% for delta, item in items %}
    <li class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
  {% endfor %}
  </ul>
</div>

Again, the field-wrapper seems like it could probably be dropped. But in both scenarios, if you do so, then you have a different markup pattern based on whether or not a label is present.

In a world with no labels, it would be nice to see that markup as:

{% for delta, item in items %}
<div class="field-wrapper {{ attributes.class }} {{ content_attributes.class }} field-item {{ item.attributes.class }}"{{ attributes }}{{ content_attributes }} {{ item.attributes }}>
  {{ item.value }}
<div>
{% endfor %}

And:

<ul class="field-wrapper {{ attributes.class }} field-items {{ content_attributes.class }}"{{ attributes }}{{ content_attributes }}>
{% for delta, item in items %}
  <li class="field-item {{ item.attributes.class }}" {{ item.attributes }}>{{ item.value }}</li>
{% endfor %}
</ul>

The real question here is how often is there going to be a scenario where somebody is going to want to write a selector based on .field-wrapper .field-item. That is the scenario that would fail if we dropped a .field-wrapper div when there is no .field-label.

I like seeing nice and tidy HTML, but I also don't want to be yelling WTF when writing CSS. Not always easy to balance out those two.

Thoughts? Suggestions?

mdrummond’s picture

Sorry, galooph, apparently I was commenting at the same time you were posting your patch, and somehow my comment deleted your patch? It seems really silly that d.o is set up to work that way. Anyhow, my apologies.

galooph’s picture

FileSize
3.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,990 pass(es), 576 fail(s), and 1,836 exception(s). View
2.56 KB

No problem, @mdrummond!

Here's my patch and interdiff from #74 again, as I think we confused the testbot.

I guess that now the tests are passing, we need to agree on the precise markup before tweaking the template again.

mortendk’s picture

@dman yes theres some inconsistencies right now (that were working on fixing) but you do know that we have been discussing this for 2 years, and no themer have so far never asked for more divitis just to have the same structure. Find me just one themer that wants 3 divs wrapping every element ;)

The one thing we have heard, world wide is that the themer wants to be able to control & structure the markup + css to what the need is for any given situation. and they wants to have defauts, that is build on sensible usecases. Not the usecase thats build on what a system wants.
The principles are summoned up https://drupal.org/node/2008464#principles
The patterns that we build yes should have the same structure, if it make sense - the same structure dosn't always apply different elements.
I have done a prototype for where we are atm http://mortendk.github.io/drupal8-fields Yes theres some inconsistences that needs to be fix (its not RTBC yet)

But going back to the divitis, is directly against what the frontend in drupal wants & needs - sorry for the rant ;)

@mdrummond i totally follow what you wanna do, im gonna do some more prototyping and suggest a structure so we reflects the elements.
having the same structure for data that have multiple elements make sense vs. the field that just have one value & no label

galooph’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,848 pass(es). View
1.11 KB

Well, the testbot really doesn't like #78 - 576 fails! Mainly '"attributes" is an invalid render array key'.

Here's the patch from #70 but with the extra space replaced. field-item{{ attributes.class }} becomes field-item {{ attributes.class }}.

mdrummond’s picture

I'll just leave this link right here so everyone can enjoy:

http://www.heydonworks.com/article/confessions-of-a-css-expert

mortendk’s picture

FileSize
3.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 713462-field-default-markup-removing-the-divitis-83.patch. Unable to apply patch. See the log in the details link for more information. View

At the sprint at midcamp chicago were working on removing the .field-item and instead have a class ,field-content that is the wrapper for the content, like the field-label for the label, the wrapper class would be field-single or field-multiple

mdrummond’s picture

For the multiple with no label, there is an extra closing div tag.

For the single with label, I don't think WTF belongs in the template.

The multiples are missing the attributes variable.

I miss having a list for multiples, but this is more consistent. If I want a list, I can easily customize the template to make that happen.

So essentially we have a different wrapper class based on whether a field is single or multiple. Interested in hearing from some of the people that had objections before if that is helpful.

For the single value field, field-content is nested in field-single if there is a field-label, while there is no nesting if there is no label. That is nice and simple: interested in hearing from those with consistency objections if this would be a concern or not.

mdrummond’s picture

I've been thinking that what might be a helpful exercise is looking through the existing CSS to see how field markup is targeted. That might provide some examples that would clarify what structure would provide the most consistent base as styling hooks.

BLadwin’s picture

FileSize
4.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,927 pass(es), 11 fail(s), and 0 exception(s). View

reroll against 8.x HEAD of patch in comment #83

mortendk’s picture

@mdrummond yes i have written up a lot here for the fields http://mortendk.github.io/drupal8-fields to make it easier to grop in our heads
btw im pretty sure wtf belongs in the template ;) - its big and complicated atm - but this is a take on actually rendering the markup to reflect what the field is supposed to do

css

  /* outer wrapper always there */
  /* single or multiple */
  .field-single{ }
  .field-multiple{ }
  
  
  /* label  */
  .field-label-hidden{ }
  .field-label-above{ }
  .field-label-inline{ }


  /* label - only there if theres a label */
  .field-label{ font-weight: bold; }

  /* content wrapper for the content */
  .field-content{ color: gray;}
  
  /* field single - specific only target the field label & field-content */
  .field-single .field-label{ color: brown; }
  .field-single .field-content, .field-single.field-content{  color:orange; }
  
  /* field multiple - specific only target the field label & field-content */
  .field-multiple .field-label{ color: burlywood ;}
  .field-multiple .field-content, .field-multiple.field-content{ color: teal;}
  
  /* wrapper for multiple items */
  .field-items{ background: lightgray;}
  .field-items .field-content{ border-bottom:1px solid green;}

4 different versions of the field beeing rendered:

single value no label

<div class="field-single field-content ... field-label-hidden">Data single value no label </div>

single value with label

<div class="field-single  ... field-label-above">
  <div class="field-label">label</div>
  <div class="field-content">Data single value</div>
</div>

multiple value no label

<div class="field-multiple field-items .... field-label-hidden">
  <div class="field-content">one </div>
  <div class="field-content">two</div>
</div>

multiple value with label

<div class="field-multiple ... field-label-above">
  <div class="field-label">label</div>
  <div class="field-items">
    <div class="field-content">data one </div>
    <div class="field-content">data two</div>
  </div>
</div>

joelpittet’s picture

@galooph re: #74 Thanks for trying that out, I was sure I had merge working somewhere before on attributes but I guess not because of that check in twig_array_merge() for is_array() and Attribute is an ArrayObject so those checks don't pass. May have to look into some solutions for that elsewhere. Also, for the removal of [delta], I'm going to move that over to a new follow-up so as to not distract from issue summary goals.

That will live over here: #2229435: Clean up the way attributes are printed in field.html.twig

Carry on:) Nothing to see here.

aboros’s picture

Status: Needs work » Needs review

There is a reroll of #83 in #87, but bot was not triggered.

mortendk’s picture

I wrote more on a suggestion for the fields markup, would be great with some comments on it (maybe we could fix the classnames at the same time ;))

-> http://mortendk.github.io/drupal8-fields

scor’s picture

@mortendk the way I read your proposal at http://mortendk.github.io/drupal8-fields is that no matter what patterns is rendered, we can be sure that each field data item is wrapped by a div with the "field-item" class, like this:

data

If that's the case, then I'm 100% on board.

camoa’s picture

@mortendk I believe that simplifies the way it works and makes easier to understand the markup or even theme the different patterns.

As scor said, the fact that we know that every data in a field has only one div and we know the class... I am so in :)

mortendk’s picture

FileSize
3.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,599 pass(es), 12 fail(s), and 0 exception(s). View

changed .field-content back to .field-item & removed some cruft
still need some heavy unit testing

dodorama’s picture

While the resulting html might look good, I feel like the template becomes a little overcomplicated with all those if, elseif and loops.
In my experience each field really wants to be treated differently and in complex cases you will end up using a template for each field if not printing the fields in the node template and using wrappers there. What I'm saying is that there are so many use cases that it might be challenging to find a solution for most of them.
In this sense, I think we might be better served by a super simple template:

  {% if not label_hidden %}
    <div class="field-label"{{ title_attributes }}>{{ label }}</div>
  {% endif %}
  {% for delta, item in items %}
      <div class="field-item{{ content_attributes }}">{{ item }}</div>
  {% endfor %}
joelpittet’s picture

@dodorama et al What about doing something along the lines of what fences module does, or maybe that's overkill but I know in d7 I include and turn that on every site. @see https://drupal.org/project/fences

Also would be nice to get input from @JohnAlbin though I'm still cool with this plan or even just splitting out multiple into it's own template as @Cottser mentioned in the twig hangout.

mortendk’s picture

Lets first agree upon the markup - then the template :)

To not end up in an bikeshed- can start with looking at markup, that this suggestion comes with, can we agree upon the data structure?

If we can agree upon the proposed structure - then lets look at how the template can work out, TBH theres no way around it, its gonna be a tiny bit complicated. But we have agreed that a themer isnt stupid - so complex problems requires a sometime complex solution - else were back at good old {{ content }} & heres all your prerendered strings Drupal took the desission for you, you cant change the markup :(

Please remember Its not the goal to make a "end all be all solutions" for all fields & all situations that drupal can throw at us - Its the goal to have a standard template that gives us markup, that don't make us vomit ;) What a themer wants specific in his theme for a specific field, is all up to the themer - The rest would be to document different versions of how a template could look.

Lets start by removing the divitis, then we find the best solution for how a template looks - and NO, no ui hell, then were back into the "the themer is dumb and dont understand anything else than the css"

more about the guidelines that were buildin on https://drupal.org/node/2008464#principles

Markup & css

http://codepen.io/mortendk/pen/rvxhw?editors=110

Template examples:

base template:
Field.html.twig: http://codepen.io/mortendk/pen/kqysv

template based on a field type
field--type.html.twig: http://codepen.io/mortendk/pen/vrzfy

Template overwrite if its a picture
field--image.html.twig: http://codepen.io/mortendk/pen/spixn

template overwrite if its a specif named field
field--name.html.twig: http://codepen.io/mortendk/pen/zjrnw?editors=100

mdrummond’s picture

What I briefly mentioned on the Twig call last night was that I wonder if a BEM syntax for class names could help resolve the concerns people have about consistency in markup.

Rather than worry about chaining together multiple selectors and using descendant selectors, we could use class names that allow people to target each element accurately no matter the markup structure.

Base component classes:

  • field__wrapper
  • field--single__wrapper
  • field--multiple__wrapper

Each of these base component classes can be used for styles regarding how this field or group of fields relate to the surrounding fields or content. These would be good classes to use to handle margin around a field for example.

As an example, it might be worth putting both field__wrapper and field--single__wrapper classes on an element: that gives somebody the choice of whether they want to have the same styles for both single and multiple value fields or if they want to vary those styles based on whether the field can handle multiple values or not.

Label element classes:

  • field__label
  • field--single__label
  • field--multiple__label

This class would have perhaps the most to gain from this class scoping. With this class, it would be simple to float a field label to the left or display above a list as appropriate.

Again a more generic field__label class allows you to handle styles that affect both styles of fields.

item element classes:

  • field__item
  • field--single__item
  • field--multiple__item

These classes allow you to style how the actual data in a field appears. You can apply the same styles for all types of field items, or you can apply specific styles for items in a multiple field context: for example, margins between each field item.

items element class:

  • field--items

There's no need for a the single/multiple variations here, because a multiple value field by its nature is the only one that could have items. This element serves as a wrapper for the items in a multiple context: probably the primary usage is for controlling the relationship between the grouping of field items and any accompanying label.

Other classes

  • field-type--bar
  • field--label-above
  • field--label-inline

I think field--label-above is a more accurate way to describe these modifiers. This is a field where the label is above. The variation seems to be label-above and label-inline. Similarly bar is a variation of field-type, not a sub element of the field-type.

Personally I would recommend avoiding the class names field--label-above and field--label-inline. Those are presentational classes, so if you want to change your label style, you need to change not only your CSS, but also your HTML.

Example: single field item, no label

<div class="field__wrapper field--single__wrapper field__item field--single__item field-type--bar">single field data </div>

Example: single field item, with label inline

<div class="field__wrapper field--single__wrapper field-type--bar">
  <div class="field__label field--single__label">single field label inline</div>
  <div class="field__item field--single__item">single field data</div>
</div>

Example: multiple field item, no label

<div class="field__wrapper field--multiple__wrapper field-type--bar">
  <div class="field__items">
    <div class="field__item field--multiple__item">data one </div>
    <div class="field__item field--multiple__item">data two</div>
  </div>
</div>

Example: multiple field item, with label

<div class="field__wrapper field--multiple__wrapper field-type--bar">
  <div class="field__label field--multiple__wrapper">label above</div>
  <div class="field__items">
    <div class="field__item field--multiple__item">data one </div>
    <div class="field__item field--multiple__item">data two</div>
  </div>
</div>

Summary
There are a few extra classes with this method, but now you never need to guess when you're writing your CSS. You don't need to know the exact markup structure for a particular field or provide variations for all the markup structure possibilities. You have classes that let you write consistent CSS, applying specific purposes to each class.

This allows us to neatly solve the divitis, provide RDF as necessary and allow for consistent styling of fields. There are a few extra classes, but in my opinion, it's worth it.

Thoughts?

mortendk’s picture

@mdrummond
i get where you at and it is powerfull, but i do think that the classes gets very heavy & im wondering if were about to try soliving 100% of all posibilities, instead of keeping it "simple" (aka less classes & easier for the eye on view-source:) and solve for the 90%
+ were still having the dobbelt wrapper for multiple fields without a label ?

i was reading up on our coding standards, and its perfectly ok to do a selector like:
.field__items > .field__item{}
vs the one classes selections ala:
.field__items--single{ }
.field__items--multiple{ }

structire

.field (field--single | field--multiple)
  .field__label (field__label--single | field__label--multiple)
    .field__items
      .field__item ( field__items--single | field__items--multiple)

i would suggest something like this:
.field--single {}
.field--multiple {}
as identifiers

.field-label--$position{}
.field-type--$type{}
.field-name--$name{}

.field__items{} as wrapper for .field__item
.field__label {}
.field__item {}

Example: single field item, no label

single field data

.field__item{}

Example: single field item, with label inline

<div class="field--single field-label--$position">
  <div class="field__label">single field label inline</div>
  <div class="field__item">single field data</div>
</div>

.field--single  > .field__label{ } 
.field--single  > .field__item{ } 

Example: multiple field item, no label

<div class="field--multiple field__items field-label--$position">
  <div class="field__item">data one </div>
  <div class="field__item">data two</div>
</div>

.field__items  > .field__item{ } 
field--multiple .field__item{ } 

Example: multiple field item, with label

<div class="field--multiple field-label--$position">
  <div class="field__label">label above</div>
  <div class="field__items">
    <div class="field__item">data one </div>
    <div class="field__item">data two</div>
  </div>
</div>

.field-wrapper--multiple  > .field__label{ } 
.field__items { }
.field-wrapper--multiple  > .field__item{ } 

As alternative to keep the amount of classes down:

Example: single field item, no label

<div class="field-wrapper--single field__item--single field-label--$position">single field data </div>

.field__item--single{ } 

Example: single field item, with label inline

<div class="field-wrapper--single field-label--$position">
  <div class="field__label--single">single field label inline</div>
  <div class="field__item--single">single field data</div>
</div>

.field__label--single{ } 
.field__item--single{ } 

Example: multiple field item, no label

<div class="field-wrapper--multiple field-label--$position">
  <div class="field__items">
    <div class="field__item--multiple">data one </div>
    <div class="field__item--multiple">data two</div>
  </div>
</div>

field__item--multiple{ } 

Example: multiple field item, with label

<div class="field-wrapper--multiple field-label--$position">
  <div class="field__label--multiple">label above</div>
  <div class="field__items">
    <div class="field__item--multiple">data one </div>
    <div class="field__item--multiple">data two</div>
  </div>
</div>

.field__label--multiple{ }
.field__item--multiple{ } 

i like the structure but i dont thikn we should have 2 classes on every item, thats a bit overkill imho. i would like to make it really easy to read as well the first time you look at the fields cource

mortendk’s picture

FileSize
5.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,656 pass(es), 12 fail(s), and 0 exception(s). View

rolled a patch based on the discussion on #100, #101 & 102

mdrummond’s picture

Talked this over with mortendk, and we're in agreement now on how to have consistent styling with a minimum number of classes while still keeping the number of divs to a minimum.

Here's our primary cast of classes:

  • field__wrapper
  • field--single
  • field--multiple
  • field__items
  • field__item

Supporting players include:

  • field-type--$type{}
  • field-name--$name{}
  • field-label--$position{}: field-label--above, field-label--inline, field-label--hidden

As our story open, there will always be at least one div. On that outermost div, you will usually want these classes:

  • field__wrapper
  • field--single or field--multiple
  • field-type--$type{}
  • field-name--$name{}
  • Our hero strides on stage, ready to take on all opponents. If there is only ever one item for a field, and it despises any labels that try to define it, then add field__item to that one sole wrapping div. That's it: success!

<div class="field__wrapper field--single field-type--hero field-name--steve field__item field-label--hidden">Steve, the heroic field item!</div>

Great, you have a nice simple field that you can consistently style. Use field__item to style the data and field__wrapper to style the relationship of this field with other fields.

What if our heroic field gets a label?

<div class="field__wrapper field--single field-type--hero field-name--steve field-label--inline">
  <div class="field__label">Steve</div>
  <div class="field__item">The heroic field item!</div>
</div>

The field__label will always be inside of a div with either field--single or field--multiple, so there is no need to provide variations within the class name itself.

Also, we're putting the label type on the wrapper div, because of the UI on the field display page, which controls this. There you can choose to display the label inline, above or to hide the label. Since a hidden label class would need to go on the wrapper div (because the label div doesn't exist), we put the other labels there too.

What if our trusty field item, Steve, gets some friends to join him, but they don't have a label yet?

<div class="field__wrapper field--multiple field-type--hero field-name--steve field-label--hidden">
  <div class="field__items">
    <div class="field__item">Steve</div>
    <div class="field__item">Tony</div>
    <div class="field__item">Bruce</div>
    <div class="field__item">Thor</div>
  </div>
</div>

In this case field__item will always appear inside field__items if there is even the possibility of more than one field item showing up to do battle. So there is no need for a variation based on field--multiple.

Finally, if our field gets a label:

<div class="field__wrapper field--multiple field-type--hero field-name--steve field-label--above">
  <div class="field__label">Avenge this!</div>
  <div class="field__items">
    <div class="field__item">Steve</div>
    <div class="field__item">Tony</div>
    <div class="field__item">Bruce</div>
    <div class="field__item">Thor</div>
  </div>
</div>

Again, the field__label will always appear within a wrapping div that tells it if this is a multiple field and the position of the field label. So no need to vary the field__label class.

The only possible tricky thing is that if you want to vary the styling of a field__item based on the field type, you would need to two selectors:

.field-type--hero .field__item,
.field-type--hero.field__item {
  font-weight: bold;
}

I don't think that is the end of the world. It may cause a little extra CSS, but it saves us HTML on every single page of the site.

I think this is a pretty good solution that offers consistency and conciseness. But I've been wrong about that before, so have at it!

mdrummond’s picture

Morten pointed out we could probably simplify this even more. For field items with no label we could have:

<div class="field__wrapper field--multiple field__items field-type--hero field-name--steve field-label--hidden">
  <div class="field__item">Steve</div>
  <div class="field__item">Tony</div>
  <div class="field__item">Bruce</div>
  <div class="field__item">Thor</div>
</div>

This is still consistent. field__item is still always inside field__items.

mortendk’s picture

Status: Needs work » Needs review
FileSize
4.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,530 pass(es), 2 fail(s), and 0 exception(s). View
4.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,558 pass(es), 2 fail(s), and 0 exception(s). View

Based on the discussions as mentioned above heres a new patch i have changed the base field name from field--wrapper to field cause it made more meaning & is shorter to read - we can always bikeshed that name ;)

field.html.twig

{% if multiple %}
  {# multiple value field  #}
  {% if label_hidden %}
    <div class="field field--multiple field__items {{ attributes.class }}" {{ content_attributes }}>
      {% for delta, item in items %}
        <div class="field__item"{{ item_attributes[delta] }}>{{ item }}</div>
      {% endfor %}
      </div>
    </div>
  {% else %}
  {# multiple value with label #}
    <div class="field field--multiple {{ attributes.class }}"{{ attributes }}>
      <div class="field__label"{{ title_attributes }}>{{ label }}</div>
      <div class="field__items"{{ content_attributes }}>
        {% for delta, item in items %}
          <div class="field__item"{{ item_attributes[delta] }}>{{ item }}</div>
        {% endfor %}
      </div>
    </div>
  {% endif %}
  {# end of multiple field  #}
{% else %}
  {# single value field #}
  {% for delta, item in items %}
    {% if label_hidden %}
      {# single value no label#}
      <div class="field field--single field__item {{ attributes.class }}"{{ attributes }}{{ content_attributes }}{{ item_attributes[delta] }}>{{ item }}</div>
    {% else %}
      {# single value with label#}
      <div class="field field--single {{ attributes.class }}"{{ attributes }}>
        <div class="field__label"{{ title_attributes }}>{{ label }}</div>
        <div class="field__item" {{ content_attributes }} {{ item_attributes[delta] }}>{{ item }}</div>
      </div>
    {% endif %}
  {% endfor %}
  {# end single value field #}
{% endif %}
mortendk’s picture

FileSize
2.6 KB

forgot the interdiff

mortendk’s picture

Status: Needs work » Needs review
FileSize
6.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,516 pass(es), 1 fail(s), and 0 exception(s). View
1.24 KB

fixed the path for the test of a terms content

falis on "Call to a member function getFieldDefinition() on a non-object" in theme.inc

  if ( isset($element['#items']) ) {
    if( $element['#items']->getFieldDefinition()->getCardinality() != "1"){
      $variables['multiple'] = TRUE;
    }
  }
mortendk’s picture

Status: Needs work » Needs review
FileSize
6.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,506 pass(es). View
1.86 KB

bot get to work

mortendk’s picture

FileSize
3.51 KB
10.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,467 pass(es). View

forgot to fix bartiks css

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs screenshots
  1. +++ b/core/includes/theme.inc
    @@ -2414,19 +2416,22 @@ function template_preprocess_field(&$variables, $hook) {
    +  if ( isset($element['#items']) && is_object($element['#items']) ){
    +    if( $element['#items']->getFieldDefinition()->getCardinality() != "1"){
    +      $variables['multiple'] = TRUE;
    +    }
    +  }
    

    All that code should be $element['#items']->getFieldDefinition()->isMultiple(); Maybe though this should be already part of core/lib/Drupal/Core/Field/FormatterBase.php::view

  2. +++ b/core/modules/system/css/system.theme.css
    index a0d527a..d4a00c8 100644
    --- a/core/modules/system/templates/field.html.twig
    
    --- a/core/modules/system/templates/field.html.twig
    +++ b/core/modules/system/templates/field.html.twig
    

    WTF! A simple template got way more complex compared to before.

mortendk’s picture

@dawehner
yes the template is a little bit more complex (its 2 if statements) than it used to be, where the logic btw was a cluster & build not for frontend use.
To solve the issue with divitis we need a tiny bit more complex solution, but as stated for years by themers, that is not a problem, else please look at the examples here: http://codepen.io/collection/GFyzJ/
Today we have a template that might is "simple" but it creates crappy markup that nobody wants.

sweet gonna fix the code for the test - the more we learn :)

mdrummond’s picture

Yes, the template is slightly more complicated, but it's pretty straightforward in how it sets up the four primary variations on field markup: multiple and singular, and label or no label.

What's more important is that it keeps the actual HTML output much more sane, with sensible class names that can be consistently targeted with CSS no matter which variation of the field markup is used.

yched’s picture

First of all, thanks a lot for kicking this, folks ! Field output is a hairy issue that always lacked the insight of real theme-oriented people, so yay !

I think I like where this is going (HTML-wise you guys should know better anyway), and the recent iterations seem to make good mood to address the concerns I had with earlier iterations (predictability on multiple values, subtle and not too telling differences between ".foo .bar" and ".foo.bar" in CSS selectors...). Double yay !

Regarding field.tpl's "increased complexity", I'm just wondering whether it should be split into separate tpls (two ? four ?), resolved at runtime (in preprocess ? in the code that sets #theme on the field render array ?).
There will still be use cases for overriding the tpl (e.g to output ul/lis, or comma-separated values), so what would be easiest for a themer to override ? One single tpl with lots of logic, or various smaller flavours ?

mortendk’s picture

Status: Needs work » Needs review
FileSize
12.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,062 pass(es), 2,453 fail(s), and 101 exception(s). View
6.52 KB

@yched yes were getting closer :)

I have been batteling the idea all day yesterday for the 20 time of how to make a "simple" template. First of all i now even more understands why we got the divitis in Drupal, its a pita to juggle the different versions.

I dont think we have to split them up in multiple version, that i think would end with a cluser of theme hook suggestions & even harder to figure out where stuff comes from.

I took a radical look today at the preprocess & tried to yank as much out of it & into the field.html.twig file & rename a couple of variables.
To create all the class names out in the template using the {% set css-class %} and renaming label_hidden to label_visible, so we can use a statement that says {% is label_visible %}, instead of more confusing {% is not label_hidden %}

instead of doing 2 loops - i have split them completely out & added in a ton of pseudo documentation, i like that way more as we explain in the theme whats going on, Anyways its a suggestion lets see where it goes. To me that would be a solution i would love -> less stuff in the preprocess, only the "hard" calculations, then let all the crap the themers want out into template

philipz’s picture

This is looking really good (except the failed test ;). The template seems easy to read and understand despite multple conditions.

One thing that I would change for sure is css-class set name to css-classes or css_classes. The set contains multiple classes so it should be named accordingly just like title_attibutes, item_attributes, content_attributes and attributes.

splatio’s picture

I've got to agree with @yched in #116 - having a single template with a huge overarching if/else doesn't sit right at all.

Yes, template suggestions are difficult for people to wrap their heads around but they are a very useful tool that themers should know how to use. Even if we did have a template suggestion for each, the 90% use cases (overriding templates and using template suggestions) would be pretty much the same.

mortendk’s picture

Status: Needs work » Needs review
FileSize
12.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,534 pass(es), 9 fail(s), and 31 exception(s). View
3.19 KB
162.84 KB

@splatio
We have experiemented with multiple theme hook suggestions at the sprint in chicago, heres where the problem with them are:
you would have to control 4 files instead of just one, which is not a thing we want, theres a wish for lesser files, especially adding "wrapper files" is a thing that very little sense, when an if/else can get the job done.

theme hook suggestions are not complicated ;)
But First theme hook suggestions are not complicated for themers to get their head wrapped around! and especially not in Drupal8, where you can see what template files there is, when you have tuned on twig_debug

But lets say we go with hook theme suggestions and split up in 4 different files:

Today with one field.html.twig

   * field--node--body--article.html.twig
   * field--node--body.html.twig
   * field--node--article.html.twig
   * field--body.html.twig
   * field--text-with-summary.html.twig
   * field.html.twig

if we view the source code with $settings['twig_debug'] = TRUE; then you get this bye viewing source:

Now instead you get this that will be printed around Every field in you theming debug, which makes it completely impossible to read where you actaully are through the view-source

<!-- FILE NAME SUGGESTIONS:
   * field-multiple-label--node--body--article.html.twig
   * field-multiple--node--body--article.html.twig
   * field-single-label--node--body--article.html.twig
   * field-single--node--body--article.html.twig
   * field-multiple-label--node--body.html.twig
   * field-single-label--node--body.html.twig
   * field-multiple--node--body.html.twig
   * field-single--node--body.html.twig
   * field-multiple-label--node--article.html.twig
   * field-single-label--node--article.html.twig
   * field-multiple--node--article.html.twig
   * field-single--node--article.html.twig
   * field-multiple-label--body.html.twig
   * field-single-label--body.html.twig   
   * field-multiple--body.html.twig
   * field-single--body.html.twig   
   * field-multiple-label--text-with-summary.html.twig
   * field-single-label--text-with-summary.html.twig
   * field-multiple--text-with-summary.html.twig
   * field-single--text-with-summary.html.twig
   * field-multiple-label.html.twig
   * field-single-label.html.twig
   * field-multiple.html.twig
   * field-single.html.twig
   * field.html.twig
-->

not to mention that you would have to rethink every time you wanted to do a change in where was that file again & what is it called ... and wtf why do i have 4 files to do one files job

I dont get the fear of 1 template file with 4 different if / else in it, can we stop with th "the themer do not understand".

field.html.twig will render all the 4 versions of a field - with good sensible markup & easy to modify classes, if you then need to modify a field lets say the a multiple image field you overwrite that single template & you would 95% know if its a multiple or single or if it haves a label or not. Not to mention IF you want to overwrite that template, its because you have a very specific need of special markup to add in, so it will have a very special need - those overwrites would still function as they are today.

for examples of this please look at the codepen i set up http://codepen.io/collection/GFyzJ/

My point is to repeat my self is that it is a complicated issue, but by having inline documentation and making it clear what we wanna do for the themer, then its not gonna be a problem, and if it is a problem now - How are we ever gonna solve the menus ;)

so please give the themers a tiny bit of credit if/else logic is not to hard to grasp (with a little bit of theme hook suggestions) - we just want the power to change stuff without having to get lost inside of drupals secret mace.

... dammit did i rant again, arg sorry :)

- there i fixed the "small" issue, that broke the test upsie
/mdk

LewisNyman’s picture

I would prefer one file over one for multiple and one for single.

yched’s picture

I myself don't have a strong opinion on the question of multiple tpl files vs one, I was just raising the question on what's easiest to override :-)

In the "separate tpls" scenario :
- I was rather thinking 2 tpls (field-single.tpl, field-multiple.tpl), each one with "if" logic for label display,
rather than 4 tpls for the combinations of single / multiple & label / no label
- Since the template would be determined by a property of the field definition ("does the field potentially accept multiple values ?"), they don't add up in template suggestions: a field is either single or multiple, so field-single.tpl & field-multiple.tpl are not eligible for the same field at the same time, and the number of actual "suggestions" (based on field name, field type...) is the same as it is today.

Just sayin'. I can totally live with "nah, one file that is basically two separate if / else branches is going to be simpler for overrides in the end".

mdrummond’s picture

I'd really rather have one template rather than two (much less four).

I have definitely run into situations where I have had a field that starts as accepting only one value, but after a while, I realize it needs multiple or unlimited values. Maybe bad planning on my part, but it happens.

In that case, if I had a custom template for that field, and we had separate templates for single and multiple fields, then the template stops working. I have to remember to create a new template then or wonder why my field no longer has the customizations that it previously (if I notice it all).

A few if-then statements with logically named variables are not that hard to understand. Certainly much easier than trying to sort out which template file to use and how to deal with that if the field cardinality changes.

camoa’s picture

I agree with one template, may look more complicated, but you read it a couple of times, with the documentation and you are golden!!

No extra divs, no extra suggestions.. life will be simpler.. I rather have to take some hard minutes understanding the template once! and not trying to find the right template or the right hook suggestion to use every time I have to theme.

mortendk’s picture

Status: Needs work » Needs review
FileSize
12.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,599 pass(es), 5 fail(s), and 0 exception(s). View
3.17 KB

fix the RDFa issues & changed back to label_hidden, cause its details theres no win in changing ;)

yched’s picture

Fine with 1 tpl then :-)

+++ b/core/modules/system/templates/field.html.twig
@@ -29,13 +29,78 @@
+{% set cssclass %}
+field field-name--{{ field_name }} field-type--{{ field_type }} field-label--{{ field_label }} {{ attributes.class }}
+    'field-' . $variables['entity_type_css'] . '--' . $variables['field_name_css'],

Nitpick :
- we tend to avoid smushing words in var names.
- the name might as well reflect that its a list of classes rather than a single class ?
- not even sure the 'css_' prefix is needed, we usually simply talk about "a class" / "classes"

Long story short: s/cssclass/classes/ ?

mortendk’s picture

FileSize
12.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,638 pass(es). View
1.43 KB
mortendk’s picture

Status: Needs work » Needs review

bot get to work

LewisNyman’s picture

Morten asked me to look over the documentation.

  1. +++ b/core/includes/theme.inc
    @@ -2439,24 +2441,15 @@ function template_preprocess_field(&$variables, $hook) {
    +  //we want to know if the field is single or multiple
    

    This is missing a space and a capital. I think this might be more clear just saying "Are there multiple field items"

  2. +++ b/core/modules/system/templates/field.html.twig
    @@ -29,13 +29,80 @@
    +We want to make it easy for the themer to remove or add css classes easy.
    +Instead of hiding these as a string in the preprocess, they are done inside of
    +the tempate file.
    

    Remove, this is useful information during implementation, but not when people are actually using the file.

  3. +++ b/core/modules/system/templates/field.html.twig
    @@ -29,13 +29,80 @@
    +{{ attributes.class }} is providing a placeholder for mddules that wants the
    +option to add classes to a field.
    

    This would be better: "{{ attributes.class }} provides a way for modules to add classes to fields dynamically"

  4. +++ b/core/modules/system/templates/field.html.twig
    @@ -29,13 +29,80 @@
    +  First we check if its a multiple value field and if it have a visible label.
    ...
    +  We check if its a multiple value field and if it have a visible label.
    ...
    +  We Check if its a single field and it have its label visbile
    ...
    +  Now check if the field is a single field and dont have a label shown
    

    I get why these are here but the are basically repeating the if statement above them? Are they really that useful? I would rather make the variables more verbose than have these comments.

  5. +++ b/core/modules/system/templates/field.html.twig
    @@ -29,13 +29,80 @@
    +  The holy grail of single field data!
    

    We don't need this either/

  6. +++ b/core/modules/system/templates/field.html.twig
    @@ -29,13 +29,80 @@
    +    <div class="field field--multiple {{ cssclass }}">
    

    This would print the field class twice?
    I think we should move {{ cssclass }} to the start of the class attribute so field--multiple isn't the first class

jjcarrion’s picture

Assigned: galooph » Unassigned
FileSize
11.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,738 pass(es). View
4.86 KB
145.43 KB

I have made the changes from the documentation that said Lewis #134, a screenshot with all the posibilities and I have removed some spaces between classes.

hope it would be usefull!

yched’s picture

The screenshot from #135 shows that we're missing some vertical spacing between successive fields ? (compare with HEAD)

Also, my remark from #128 ("{% set cssclass %}") still stands ?

dodorama’s picture

I still believe that, as suggested in #97, we are overdoing a little.

The template is getting more complex. If I'm a themer and I'm looking at the template for the first time it doesn't really makes me want to mess with it.
The HTML output is going from divitis to classitis. Are we trying to serve too much use cases with all those classes? When do we really need a class to distinguish single and multiple fields? Wouldn't it be better to make those variables available for the themers to use in the template but don't actually print them on the default template?

We need to consider that the first approach for most themers would be to scan the html code. If I look at the screenshot right now, I perceive a complexity with all those long classes that gives me the wrong impression. We need to understand that the default template shouldn't be a template to rule them all, but a good example that serves 80% of use cases and makes it easy to build on top of it.

mortendk’s picture

@dodorama
True theres alot of classes right now, its as an example of how to do it based on the current ton of classes that Drupal7 have. Theres another issue open for this #2214249: Fields classes - what do we actually need & want in Drupal8 twig
- so yup that needs to be cleaned up.

On the level of complexety & if its to hard for a themer to understand - sorry but i dont agree, I do think its interesting that "themers" arent complaining about it, but "developers" are ;) - themers arent that fragile ... i hope.

I think it would be good to see if theres some themers that dont want that control it all , if s/he opens up field.html.twig.
cause honestly if you wanna open up field to change default behaviour, then you really wanna go to town on it.

if you want to do an overwrite on a specific field, you allready know about it. if its single/multiple etc

cheers (its saturday ;) )

mikl’s picture

If a person has enough technical chops to understand HTML and CSS, I don't think we need to worry about simple if/else/elseif logic in templates.

At least, I've never heard complaints from the frontend people I've worked with on this. And I've worked with a lot of weird people, present company included ;)

LewisNyman’s picture

It's important to bear in mind that the markup output is also an interface for understanding what's going on. If you have some styling added somewhere and you look in a mark up inspector to find out where it's being applied, div soup makes it very difficult to understand. Removing complexity in the template adds complexity somewhere else.

dodorama’s picture

I guess my main concern is with the number of classes and I understand there's a different issue for it.
In terms of markup I wonder if we need the "field__items" wrapper for multiple with label.
And more in general, I'm not a big fan of generic wrappers. In my, obviously limited, personal experience when I'm dealing with complex fields, I go and override the template anyway. That's why I was suggesting to get rid of wrappers all together and get a simple straightforward template and markup.

But I understand there might be somebody happy with a generic wrapper. I'm probably too much of a markup purist :)

mikl’s picture

#141: Well, either we'll need the field name on both field value and field label, or we'll need a wrapper around both.

I don't think it's that unreasonable to have a wrapper, it'll make the out-of-the-box markup a lot more useful (ie. being able to float the value to the side of the label).

LewisNyman’s picture

But is the field item wrapper a common use case? I don't know, you already have a wrapper around the entire field, that's more useful. Is it really easy to add in if you want it? It is.

mortendk’s picture

* field__items wrapper for a bunch of field_item, is needed as long as we have the inline/above/hidden label ui selector. in short: else inline will only be inline for the first field__item, and the other field__item will float below the inline label.

@lewis field__item is a vey normal usecase and it needed for RDFa.
if the field is a single with no label its just the div.field__item dont see the problem here.

its documentated multiple times in the different usecases that we have shown, please go look at the if in doubt about the markup patterns:

if theres concerns about these, then please be specific, else were just going in circles & not moving anywhere.

I suggest we move forward & get some screenshots running for every instance there might be of fields.

mdrummond’s picture

I think we have the markup and classes in good shape. Let's move this forward to RTBC.

joelpittet’s picture

Status: Needs review » Needs work
FileSize
203.65 KB
+++ b/core/modules/system/templates/field.html.twig
@@ -29,13 +29,72 @@
+      <div class="field__item"{{ content_attributes }}{{ item_attributes[delta] }}>{{ item }}</div>

Unless you can merge ArrayObjects (attribute.class) or objects that act like arrays(attribute) in Twig... this will just cause duplicate attributes. Also if you do merge attributes you need to know which attribute wins out in the duplicate attribute name (id="field--1" vs id="field__content--1" vs id="field__item--1")for a contrived example).

For the merging you can do that in preprocess with little resistance ATM, I know that may not be ideal but at the moment that's the only feasible way. But you still have the who wins in the merge.

FTR, I'm completely pro having "more attributes in the template and less everything in the preprocess" as an end goal here, just want to bring some reality to this experiment.

So for a way forward: If we can solve the issue of merging and other array manipulations(slice,sort,first,last,etc) for twig templates with non-arrays (AttributeArray and Attribute) and we deal with the "who wins" issue in either case this issue can have legs. Otherwise it's this guy:

Also, side note, I was really close at getting AttributeArray aka {{ attributes.class }} to be just a normal array, though currently that is borked because printing {{ attributes.class }} knows it's an array and tries to think it's a renderable array. @see #2239945-7: Refactor Attribute class into one class

mortendk’s picture

Status: Needs work » Needs review
FileSize
3.54 MB
3.54 MB
4.96 KB
11.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,163 pass(es), 1 fail(s), and 0 exception(s). View

changes in this patch:
* removed ·field--single / .field--multiple theres no 80% usecase for it (and one less class to drag around -yeah!)
* changed the classnames from .field-name--[fieldname] to .field--name-[fieldname] cause they are modifiers to the .field and not to field-name
* added ":" after the field label, cause thats not a thing we are changing in this patch.
* removed class from {{ attributes|without('class') }} cause of #2227869: Remove magic printing from Attribute class to make Twig 'without' filter work for attributes made it into core - so no more hidden magic ;)
* removed float:left instead using display: inline-block that gives the added bonus of not having to use clearfix on each field.

screenshots added - but they are huge, so click on them

This patch do not take care of the issue raised in #146 :/

yched’s picture

  1. +++ b/core/modules/system/templates/field.html.twig
    @@ -17,25 +17,90 @@
    +  <div class="{{ classes }}field--multiple">
    

    leftover field--multiple ? (it's only within a comment)

  2. +++ b/core/modules/system/templates/field.html.twig
    @@ -17,25 +17,90 @@
    +{% elseif not multiple and not label_hidden %}
    ...
         {% for delta, item in items %}
    

    Seems confusing to keep a foreach in the "not multiple" case ?
    deltas are supposed to be sequential ints starting at 0, so items[0] should be safe here ?

  3. +++ b/core/modules/system/templates/field.html.twig
    @@ -17,25 +17,90 @@
    +{% elseif not multiple and label_hidden %}
    ...
    +  {% for delta, item in items %}
    

    Same

mortendk’s picture

Status: Needs work » Needs review
FileSize
14.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,106 pass(es), 19 fail(s), and 1 exception(s). View
6.75 KB

this patch:
* fixed the test (hopefully) failed cause of changing in classnames
* changed the for loops for single value items

Attributes merging
Began to work on the merging on attributes - but my skills in Drupal8 & how to get data out is very low, so any help with some array merge recursive would be apprciated.
The patch here has some dummy data added in the preprocess that yes will be removed asap (the same with the ugly classes)

On the how do we handle multiple data in the prepreprocess, i think we should just add it all in there so if theres multiple roles, when they get in there role="foo bar baz" same with classes class="foo bar baz" or *gasp id="foo bar" If modules provides multiple id's whatever. then it must be that modules role to sort out how it will handle it.

mortendk’s picture

FileSize
10.07 KB
15.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,779 pass(es), 1 fail(s), and 695 exception(s). View

Finally found a way to merge arrays ... not sure if its the right way to do it, but it gets the shit done :)

there :

function mergeAttributes($array) {
  $mergedAttributes = array();
  foreach(func_get_args() as $argument) {
    foreach($argument as $key => $value) {
      if (!isset($mergedAttributes[$key]) ) {
        $mergedAttributes[$key] = array();
      }
      if (is_array($value)) {
        foreach($value as $innerValue) {
          $mergedAttributes[$key][] = $innerValue;
        }
      } else {
        $mergedAttributes[$key][] = $value;
      }
    }
  }
  return $mergedAttributes;
}

$variables['attributes'] =  mergeAttributes ($variables['attributes'], $variables['content_attributes'], $variables['item_attributes']['0']);
mortendk’s picture

Status: Needs work » Needs review

bot get to work

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Tested it local with patch #2231505: Convert theme_field__node__title() to Twig thats RTBC & waiting -> passes into green :)

rooby’s picture

Your merging function seems to work like php's array_merge_recursive()

It also makes all attributes an array, but we need to allow for non-array attributes right?

I haven't had a chance to actually test your earlier version of the patch but what was wrong with the output from NestedArray::mergeDeep()?

mortendk’s picture

the thing that was wront with nestedArray::mergeDeepArray() was that it only left the last part of the array - so no nesting just overwrite, my lack of knowledge here in OOP & D8 is to big to figure out if i was doing it wrong, so this totally needs some work one way or another

afaik all attributes are arrays ??

rooby’s picture

There are currently classes for array, string, and boolean attributes.

Single value attributes like id would be strings.

I thought one of the intended uses of nestedArray::mergeDeepArray() was for attributes, so I'll test out the previous patch a bit later today and see if I can work out what's going on.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
19.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,588 pass(es), 2 fail(s), and 1 exception(s). View
18.92 KB

@rooby and @mortendk this isn't perfect by any means but should get you a bit closer and yes @rooby deepMerge is the way to go. Only trouble is some things are arrays and others are ArrayObjects with AttributeBoolean/AttributeArray/AttributeString values in it, so merging becomes tricky. Cross my fingers on this though.

joelpittet’s picture

mortendk’s picture

@joelpittet dosnt work :(
its not merging the attributes instead its overwriting the values with the last added value - test it by uncommet the dummy test in theme.inc with the preprocess *sadness* Dont know it its the NestedArray::mergeDeep function thats not working prober or ?

camoa’s picture

Ok, I am trying to apply #162, and it was impossible, apparently is making the same change twice somewhere, I believe.... I'll try again and see what I can see on why the function is not working.

LewisNyman’s picture

Issue tags: +frontend, +html
joelpittet’s picture

FileSize
19.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,784 pass(es), 5 fail(s), and 1 exception(s). View
joelpittet’s picture

Status: Needs work » Needs review

I think we need to move the attributes to the preprocss, and leave the markup of the divs to the template. IMO

mikl’s picture

Fixing the tests…

camoa’s picture

@mortendk and @joelpittet Working on the merge, MergeDeep is not gonna make it, mergedeep will actually only add value of the last array passed for each key, unless those values are arrays themselves (if that makes sense) Basically the reason is failing is because that is the intended use of this, in my understanding.

The proposal is: Keep the attributes separated and use them as variables in TWIG, this way you put your attributes as you want and have more control, just merge the classes.

This can be done with a simple array_merge() and some test to make sure we do not pass a NULL value, or it returns NULL.

Another consideration, and the reason I don't submit a path, would be about the decisions being made about the theming layer in general and maybe moving clases to the template and out of the preprocess.

Any comments? what are the use cases where we need attributes merged? what concerns on RDF? Please comment so we can move forward.

Thanks

mikl’s picture

Status: Needs work » Needs review

167: 2214241-field-divitis-167-reroll.patch queued for re-testing.

The test run locally, so it seems to be some stupid test bot thing…

mikl’s picture

Okay, it looks like all the test failures are caused by missing classes on fields – for example, with the #167 patch, the user picture is output like this:

<div class="field-items">
  <div class="field-item">
    <a href="/user/1">
      <img class="image-style-thumbnail" src="http://d8.d7.hoegh.co:8080/sites/default/files/styles/thumbnail/public/pictures/Screen%20Shot%202014-05-26%20at%2011.23.38.png?itok=hESC4Hn2" alt="" typeof="foaf:Image" height="100" width="87">
    </a>
  </div>
</div>

Since there is no identifying class on the field, the test cannot verify that it's output. I guess there should be at least a `field--name-user-picture` or similar somewhere…

yched’s picture

Did this stall for good ? Would be sad, this was really promising :-/

Also, pinging @mortendk about #2229435: Clean up the way attributes are printed in field.html.twig - not sure that conflicts with the approach taken here.

mortendk’s picture

nope but were fixing the banana issues first then were gonna get the classnames n stuff in that way instead.

Manuel Garcia’s picture

tonystar’s picture

Issue tags: +theme system cleanup
lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

FileSize
8.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,769 pass(es), 170 fail(s), and 378 exception(s). View

Rerolled

hkirsman’s picture

FileSize
12.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,778 pass(es), 170 fail(s), and 377 exception(s). View

So I synced css from 2214241-field-divitis-167-reroll.patch

1. removed the debug code:
* @todo: remove these test classes for attributes merging */
...

2. there's no inline classes in html. But the css will stay so fix it from backend:
.field--label-inline .field__label,
.field--label-inline > .field__item,
.field--label-inline .field__items {

3. So why do you overwrite only 2 styles for rtl here:
[dir="rtl"] .field--label-inline .field__label,
[dir="rtl"] .field--label-inline .field__items {

This is the code before rtl:
.field--label-inline .field__label,
.field--label-inline > .field__item,
.field--label-inline .field__items {

So in rtl one does not overwrite ".field--label-inline > .field__item,"

I didn't test .field__items. Where can I see that? Maby it's not in D8?

4. Where can I test this:
submitted .field--name-field-user-picture img {

5. Anyways, I didn't check anything in the core/themes/bartik/css/style.css
except;
.field--type-taxonomy-term-reference,

apratt’s picture

Here is the interdiff for the previous patch.
There is a trailing space error in the patch.

mortendk’s picture

Status: Needs work » Needs review
Issue tags: +drupalhagen
lauriii’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
12.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,622 pass(es), 29 fail(s), and 0 exception(s). View

This should fix some notices

lauriii’s picture

mjozoff’s picture

I am making screenshots.

LewisNyman’s picture

Assigned: lauriii » Unassigned
Issue tags: +Needs reroll
jussil’s picture

Assigned: Unassigned » jussil

I will recreate the patch.

sushilkr’s picture

Status: Needs work » Needs review
FileSize
8.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Field/WidgetBase.php. View

Rerolled.

amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +SprintWeekend2015
FileSize
8.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,441 pass(es), 3,688 fail(s), and 1,412 exception(s). View
5.34 KB

"Invalid PHP syntax in core/lib/Drupal/Core/Field/WidgetBase.php." issue fixed along with formatting issues which were coming while applying the patch.

yched’s picture

The indentation in field.html.twig looks off - is that intended ?

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
9.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,562 pass(es), 1,928 fail(s), and 1,400 exception(s). View
3.38 KB

Indentation fixes in field.html.twig as per #196.

lauriii’s picture

Assigned: jussil » Unassigned
lauriii’s picture

Status: Needs work » Needs review
Issue tags: -drupalhagen
FileSize
9.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,522 pass(es), 26 fail(s), and 0 exception(s). View
2.76 KB

Lets see if this fixes tests

lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

FileSize
115.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,408 pass(es), 23 fail(s), and 3 exception(s). View

Rerolled the patch. Lets see what testbot says

lauriii’s picture

Status: Needs work » Needs review
b0unty’s picture

Status: Needs work » Needs review
FileSize
9.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,477 pass(es), 22 fail(s), and 0 exception(s). View
1.71 KB

fixed a fatal + the three exceptions.. lets see what happens

Xano’s picture

Issue summary: View changes
Xano’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1538,9 +1538,15 @@ function template_preprocess_field(&$variables, $hook) {
    +   // Are there multiple field items.
    

    This is written as a question, but should say something like Check if there are multiple field items.

  2. +++ b/core/includes/theme.inc
    @@ -1552,6 +1558,18 @@ function template_preprocess_field(&$variables, $hook) {
    +  // Merge the attributes when its a multiple fields with hidden label
    

    its should be it's (from it is).

  3. +++ b/core/includes/theme.inc
    @@ -1552,6 +1558,18 @@ function template_preprocess_field(&$variables, $hook) {
    +  // Merge the attributes when its a multiple fields with hidden label
    

    Comments need to end in a period.

  4. +++ b/core/modules/system/css/system.theme.css
    @@ -559,19 +559,20 @@ ul.tabs {
    diff --git a/core/modules/system/templates/field.html.twig b/core/modules/system/templates/field.html.twig
    
    +++ b/core/modules/system/templates/field.html.twig
    @@ -28,6 +28,9 @@
    + * - field_type: @todo: needs description
    + * - field_name: @todo: needs description
    + * - field_label: @todo: needs description
    

    These might be copied from the original PhpTemplate code. See https://www.drupal.org/files/1898062-field-module-92.patch.

b0unty’s picture

Assigned: lauriii » Unassigned
Issue tags: +Needs tests

tagged for tests

lauriii’s picture

Issue tags: -Needs tests

I don't think we need extra test coverage because of this change

b0unty’s picture

Status: Needs work » Needs review
Issue tags: +drupaldevdays
FileSize
14.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,388 pass(es), 4 fail(s), and 0 exception(s). View
8.92 KB

modified the template a bit for clarity and so that it works like its intended & Fixed some tests

b0unty’s picture

Drupal\field\Tests\EntityReference\EntityReferenceFormatterTest fails because of markup has changed, but also fails because its missing a field--name- class { not rendered } (?)

as for the rdf test i tried to debug what's going wrong but couldnot find it.

also forgot an empty line from the template end

b0unty’s picture

Status: Needs work » Needs review
FileSize
16.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,389 pass(es), 3 fail(s), and 0 exception(s). View
2.32 KB

Drupal\field\Tests\EntityReference\EntityReferenceFormatterTest

lauriii’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/templates/field.html.twig
@@ -86,4 +86,4 @@
\ No newline at end of file

New line needed after every file

b0unty’s picture

its just on the old patch, anyways the tests will fail so might aswell leave it needs work

b0unty’s picture

  // Merge the attributes when its a single field with hidden label
  if ($element['#label_display'] == 'hidden' && !$variables['multiple'] && is_object($element['#items'][0])) {
    $variables['attributes'] = NestedArray::mergeDeep($variables['attributes'], $variables['content_attributes'], (array) $element['#items'][0]->_attributes);
  }

this block fails the tests, the merge doesnt work properly

Failing test debug values

Without merge :

array (size=2)
'type' => string 'uri' (length=3)
'value' => string 'http://d8.dev/user/2' (length=20)

With merge :

array (size=2)
'type' => string 'uri' (length=3)
'value' => string '_:genid1' (length=8)
nlisgo’s picture

The tests are failing but before we attempt to address whether they should be passing or not we should verify that the markup that is generated is as expected.

I installed D8 and created an article node and went to the front page as this would display the article in the teaser view mode which seems to be where the tests are failing.

I am supplying an interdiff which shows the effect of applying the patch on the markup of the node.

This will allow someone to verify that the patch is having the expected result.

The most interesting thing for me is there seems to a duplicate of each of the following rdf properties: schema:dateCreated, schema:author and schema:text. Are those duplicates expected?

nlisgo’s picture

Here is the html output for a test article node in the teaser view after the patch is applied.

dman’s picture

That's a handy aspect to test. And no, duplicates in the RDF properties are certainly not to be expected or desired.

In this case, the second date (as a "rdf-meta hidden") is totally redundant as the better, visible, date is where the attribute should be. I presume the meta-hidden came in as previously the printed date was not being annotated correctly.

From looking at the second output of the actual HTML, the schema:text is actually fine (not a dupe, just shifted).

But the schema:title has a similar problem to the schema:date - it's now rdf-meta hidden, where it should just be applied to the h2, and not hidden. Before patch was better in that case.

So yeah, the result we are getting with that patch is pretty screwed up markup-wise.

b0unty’s picture

I wonder what was the idea behind the deep_merges initially as it seems now they only break things.

b0unty’s picture

Issue tags: +Needs reroll
lauriii’s picture

I didn't have much to say but beside the fact we still need to fix the failing tests, reroll the patch, there is still some nits that might be good idea to solve too :)

  1. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php
    @@ -183,17 +183,15 @@ public function testEntityFormatter() {
    +        <div class="field__items">
    +              <div class="field__item">' . $this->referencedEntity->label() . '</div>
    +            </div>
    +  </div>
    

    This could be refactored to be something else.. This is really nasty.

  2. +++ b/core/modules/system/css/system.theme.css
    @@ -559,19 +559,20 @@ ul.tabs {
    +  padding-right: 0.5em;
    

    This is missing /* LTR */ comment

  3. +++ b/core/modules/system/templates/field.html.twig
    @@ -28,6 +28,9 @@
    + * - field_type: @todo: needs description
    + * - field_name: @todo: needs description
    + * - field_label: @todo: needs description
    

    There is still open @todo comments on the patch

Manuel Garcia’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,266 pass(es), 3 fail(s), and 0 exception(s). View

Reroll of #215

davidhernandez’s picture

Adding this to the meta we have for removing divitis from other templates so it is easier to keep track of.

mortendk’s picture

yes the merging of attributes is the last BIG one, so we should really push whomever we have on this, to fix the merging of attributes

lauriii’s picture

Assigned: Unassigned » lauriii

Lets see WTF is wrong with this.

lauriii’s picture

Status: Needs work » Needs review
FileSize
16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,537 pass(es). View
689 bytes
Manuel Garcia’s picture

Whohoo! lauriii++

mortendk’s picture

Issue tags: -html, -SprintWeekend2015, -drupaldevdays +Bartik, +Seven, +classy
FileSize
32.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,493 pass(es), 11 fail(s), and 0 exception(s). View

fixed the classnames so they are aligned with the bem naming & all that
... Lets se how much that break

lauriii’s picture

Could you post a interdiff? It would make it easier to figure out what broke the tests..

b0unty’s picture

FileSize
21.54 KB

interdiff betweeen 231 and 233

b0unty’s picture

Assigned: lauriii » Unassigned

unassigning

b0unty’s picture

+++ b/core/modules/system/templates/field.html.twig
@@ -38,30 +41,43 @@
-{%
-  set classes = [
-    'field',
-    'field-' ~ entity_type|clean_class ~ '--' ~ field_name_class,
-    'field-name-' ~ field_name_class,
-    'field-type-' ~ field_type|clean_class,
-    'field-label-' ~ label_display,
-    label_display == 'inline' ? 'clearfix',
-  ]
-%}

Why were these removed?

davidhernandez’s picture

I don't quite understand what is going on here. Looking at the last few patches, I see variables with strtr() being reintroduced that we purposefully removed in previous issues (which negates needing clean_class in the templates,) additional preprocessing being added, and field templates that are 10 times bigger and more complicated than they are now. Isn't the goal to simplify things? I'm only looking at the patches, so I haven't looked at the markup this produces, but it doesn't look like this will reduce div usage or nesting much.

Looking at some of it, especially the reintroduction of the preprocess elements, this looks to be picking up from work done a long time ago. Much of it starting in #160 (which was a year ago,) but it doesn't reflect some of the changes (including approaches) we've made elsewhere. Maybe this requires taking a step back?

Also, I don't see <ul> being used as the summary suggests, so could at least use a summary update?

mortendk’s picture

@david We have had that conversation about the field template multiple times one of the goals is not to simply things to "protect the themer" its more important that we can find & figure out where stuff is coming from and why its there, even that the template gets bigger. But if you look at it it makes loads of sens, and IF we dont change the template we wont kill divitis and it will be the same div hell that comes out.

i dunno about all the other things in the background ;)

davidhernandez’s picture

@morten, and part of what I'm not clear on is how this is killing divs? For one thing, tests aren't being changed much outside of changing class names, so that seems indicate the markup isn't moving much.

Can someone post before and after screenshots of the markup so we have a good comparison?

mdrummond’s picture

+++ b/core/modules/system/templates/field.html.twig
@@ -49,19 +52,38 @@
+{% else %}
+  {% if multiple %}
     {% for item in items %}
-      <div{{ item.attributes.addClass('field-item') }}>{{ item.content }}</div>
+      <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
     {% endfor %}
-  </div>
+  {% else %}
+      <div{{ content_attributes.addClass('field__items') }}>
+      {% for item in items %}
+        <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
+      {% endfor %}
+      </div>
+  {% endif %}
+{% endif %}

The logic here is reversed.

field_items is being shown if there aren't multiple field items.

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
mortendk’s picture

Status: Needs work » Needs review
FileSize
15.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,569 pass(es), 2,668 fail(s), and 1,059 exception(s). View
10.91 KB

found an issue where i coundt get the theme to understand if there was multiple fields :(
also it needs love towards core, but lets make classy work first

lauriii’s picture

Didn't dig into the failures, but there seems to be a fatal error which might be good to solve first. It might solve most of the test failures.

Here's some random points:

  1. +++ b/core/includes/theme.inc
    @@ -1523,9 +1523,15 @@ function template_preprocess_field(&$variables, $hook) {
    +  $variables['multiple'] = FALSE;
    +  if (isset($element['#items']) && is_callable($element['#items'], 'getFieldDefinition') && is_callable($element['#items']->getFieldDefinition(), 'isMultiple')) {
    +    $variables['multiple'] = $element['#items']->getFieldDefinition()->isMultiple();
    +  }
    
    @@ -1537,6 +1543,18 @@ function template_preprocess_field(&$variables, $hook) {
    +  // Merge the attributes when its a multiple fields with hidden label
    +  if ($element['#label_display'] == 'hidden' && $variables['multiple']) {
    +    $variables['attributes'] = NestedArray::mergeDeep($variables['attributes'], $variables['content_attributes']);
    +  }
    +  // Merge the attributes when its a single field with a label
    +  if ($element['#label_display'] != 'hidden' && !$variables['multiple'] && is_object($element['#items'][0])) {
    +    $variables['content_attributes'] = NestedArray::mergeDeep($variables['content_attributes'], (array) $element['#items'][0]->_attributes);
    +  }
    +  // Merge the attributes when its a single field with hidden label
    +  if ($element['#label_display'] == 'hidden' && !$variables['multiple'] && is_object($element['#items'][0])) {
    +    $variables['attributes'] = NestedArray::mergeDeep($variables['attributes'], $variables['content_attributes']);
    +  }
    

    This is not needed in Drupal Core I assume? Could we move this to Classy? We need to also consider how this affects RDF and how that should be tackled

  2. +++ b/core/modules/system/templates/field.html.twig
    @@ -49,19 +52,38 @@
    +    <div{{ content_attributes.addClass('field__items') }}>
    +      <div{{ title_attributes.addClass(title_classes) }}>{{ label }}:</div>
    +    {% for item in items %}
    +      <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
    ...
    +      <div{{ title_attributes.addClass(title_classes) }}>{{ label }}:</div>
    ...
    +        <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
    ...
    +      <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
    ...
    +      <div{{ content_attributes.addClass('field__items') }}>
    ...
    +        <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
    

    These left overs should probably be removed too

  3. +++ b/core/modules/system/templates/field.html.twig
    @@ -49,19 +52,38 @@
    +  {% if multiple %}
    

    Is this really needed if we dont add any classes?

  4. +++ b/core/themes/classy/templates/field/field.html.twig
    @@ -28,38 +28,75 @@
    + * - field_type: @todo: needs description
    + * - field_name: @todo: needs description
    + * - field_label: @todo: needs description
    

    @todos needs to be done after we get a green patch

  5. +++ b/core/themes/classy/templates/field/field.html.twig
    @@ -28,38 +28,75 @@
    +{#
    +The field template is divided into 4 different variations all depending
    +on how the field is configured.
    +We create 4 variations of the markup to prevent divits & make it easier to
    +modify later on
    +* Label & multiple fields
    +*
    +#}
    

    Not sure if I'm right since I didn't find any reference for this but I think this should be still inside /** */ like in PHP to make it look a little cleaner

  6. +++ b/core/themes/classy/templates/field/field.html.twig
    @@ -28,38 +28,75 @@
    +    {# single item field with no label #}
    

    s/single/Single and period to the end

  7. +++ b/core/includes/theme.inc
    @@ -1523,9 +1523,15 @@ function template_preprocess_field(&$variables, $hook) {
    +  if (isset($element['#items']) && is_callable($element['#items'], 'getFieldDefinition') && is_callable($element['#items']->getFieldDefinition(), 'isMultiple')) {
    

    This should be split into multiple ifs probably

mortendk’s picture

@laurii yes we wanna still kill the divitis in core as well, so the split between single & multiple would still be needed I think

We first need to fix all the test that are breaking + fix the original issue that is: "how do we combine the attributes for each element when we merge them"

If we dont fix them what will happen is that themers will overwrite the divitis crappy field template & not caring about whatever rdf is doing.

So we have to fix this issue else drupal will keep beeing a divitis hell, thats killing drupal from the inside...

ShaunDychko’s picture

Issue summary: View changes
ShaunDychko’s picture

Issue summary: View changes
ShaunDychko’s picture

Assigned: Unassigned » ShaunDychko
ShaunDychko’s picture

I think it's helpful to refer to the Fences module for a bit of guidance as to what the default field markup should be. The module was written by JohnAlbin who is the creator of the Zen theme. When using a div the D7 default markup for a multi-value field using Fences is:

<h3 class="field-label">Test Label </h3>
<div class="field field-name-field-test-label field-type-text field-label-above">
    Some text  
</div>
<div class="field field-name-field-test-label field-type-text field-label-above">
    Second line of text  
</div>

There is no wrapper, and the template files are simple: the label is conditionally printed if it's not empty, and a loop prints each value in it's own div. This is as lean as it gets, and shouldn't that be the best starting point for core templates?

If a wrapper is needed for a particular field, it would be a simple process of overriding the core template in the custom theme for that specific field. Most fields don't need a wrapper, and the core template should meet this most common use case. In any case, the wrapper is still present if any "attributes" are set. The Quick Edit module currently sets a data-quickedit-field-id attribute, so the wrapper is still printed when the Quick Edit module is installed. However, when Quick Edit is uninstalled, the Gold Standard of lean markup a-la-D7-with-Fences is output.

It seems a bit clunky testing whether "attributes.storage" is empty in field.html.twig instead of testing "attributes", but I found that even when there are no attributes to print, the variable "attributes" always contains an empty "storage" array and I guess the empty test isn't recursive, so "attributes" is always not empty since it contains an array.

Let's modify the core field.html.twig used in the Stark theme first since it's easier to get the most basic version of the template correct first, then incorporate the changes into Classy.

With the wrapper generally not being used, all the classes need to move to the field item. I renamed some of them using https://www.drupal.org/coding-standards/css/architecture#extend for guidance. I added classes to the "field-label" since without the wrapping div the labels lose their context. If a themer wants to target all labels of a particular field-name, for example, they'll need these new classes to do so. The CSS guidance says to create class names to identify components. There are two components here: a label and an item, so "field-label" and "field-item" seem like good component names. Variations are separated by double dashes, and when the field-item component refers to a variation of the field-label component, that got the class name "field-item__field-label--above", with the two component names separated by double underscores.

Here are some screenshots.

Original Markup before changes:

before

Markup After, with Quickedit installed which makes the wrapper get printed:

after, with quickedit.

Markup After, with Quickedit Uninstalled (nice and lean!)

after, no quickedit.

There were some concerns in the comments above with RDFa, so I tested that on the comment body. There isn't a problem since the RDFa attributes are included in the attributes of each field item. Screenshot: rdfa in comment field

I couldn't find any module that makes use of the "content_attributes" variable in field.html.twig, so it seems safe to delete the "div.field-items" wrapper. The "content_attributes" variable is included in every template by "_template_preprocess_default_variables()" at 1214:core/includes/theme.inc and it doesn't seems to have any special purpose in field.html.twig.

Some follow-up issues could include making the Quick Edit module put it's custom attribute on field items instead of on the wrapper, in which case the wrapper would no longer be printed. Alternatively, the Quick Edit module could check if someone is logged in with access to edit the field, and only then would it print the attribute. Currently the attribute is printed even for Anonymous users.

Fingers crossed for passing tests...

ShaunDychko’s picture

Status: Needs work » Needs review
ShaunDychko’s picture

Assigned: ShaunDychko » Unassigned
mortendk’s picture

Status: Needs review » Needs work

@ShaunDychko
We have to do all this on classy - stark dosn't matter ;)
* All test is done on the classy theme, this is where the default markup & css classes lives. Stark should have no classes the only classes that should ever live there is a class that is essential for functionality (hidden/visibile classes & js- prefixed for javascript)
* Uninstalling Quickedit is not an option to kill divitis, so we need to merge the attributes on that outer wrapper.

The markup and css needs to be predictable, removing the wrapper if there's not a label on the field will break our BEM/OOCSS structure.

Classnames can be changed but we really dont want the class soup of Drupal 7 its by design that we want these to be as few as simple.

yched’s picture

Big +1 to @mortendk in #255 :

The markup and css needs to be predictable, removing the wrapper if there's
not a label on the field will break our BEM/OOCSS structure.

The markup that was implemented in earlier versions before #252 was the result of months of discussions, really not sure we want to re-open that debate :-)

ShaunDychko’s picture

Thanks @mortendk and @yched for the great feedback. Yeah, you're both totally right that the markup needs to be predictable. Having a wrapping div pop up when an attribute happens to be added isn't predictable, so that needs to change.

As for:

The markup that was implemented in earlier versions before #252 was the result of months of discussions, really not sure we want to re-open that debate :-)

Since I'm new to the issue, I have a bit of stamina. :) On the other hand, if there's no appetite for talking about this further, I can totally respect that, but reconsidering the markup solves some tricky issues here. From comment #146 to #245 there are multiple problems concerning the merging of attributes. #222 mentions duplicate RDFa attributes, #229 mentions that merging is the BIG issue, and there isn't a passing patch until #231. #239 then critiques the patch in #231 mostly on the grounds that the template is complicated (I totally agree!). Then by #242 there's a realization that

The logic here is reversed.

field_items is being shown if there aren't multiple field items.

and #245 mentions that a case was found where the theme couldn't understand if there were multiple items or not. By having the proposed solution below, we avoid the need both for merging arrays, and for checking whether multiple field items exist.

Proposed Solution

Get rid of the "div.field-items". This removes one level of depth to get to the field item, so helps with "divitis". Unfortunately it doesn't remove the outermost wrapping div, but that div is being used by other modules, such as Quick Edit, so removing it might be problematic in ways that are difficult to identify. The only comment I could find to suggest keeping div.field-items is that when a label is selected to display inline, then a field with multiple values will have values after the first fall under the label. That requires two "ifs" (label inline, multiple field values), so couldn't that be a special case that the themer needs to deal with in their customization? It seems like "content_attributes" is never used for fields, and keeping those is the only other reason I can imagine needing div.field-items of a core template. If div.field-items is needed, lets document how to make that easy customization.

This suggestion makes a compromise between aiming for the perfect markup, which has complexity resulting in a difficult to understand template and failing logic in the patches, vs having a simple guaranteed-to-work solution that reduces divitis, even though it doesn't reduce is maximally.

This suggestion follows the model of the Fences module, also mentioned in #98, but adds a wrapping div since Quick Edit needs it (but that could be changed? Naw, nevermind, let's not go there), so I think the markup will work nicely.

Removing div.field-items results in a super simple template that every themer can understand, the gist of which is:


<div{{ attributes }}>
  {% if not label_hidden %}
    <h3{{ title_attributes.addClass(title_classes) }}>{{ label }}</h3>
  {% endif %}
  {% for item in items %}
    <div{{ item.attributes.addClass(classes) }}>{{ item.content }}</div>
  {% endfor %}
</div>

So, how about it? Shall I roll another patch in this direction?

ShaunDychko’s picture

Out of curiosity I removed the remaining wrapping div and put the "data-quickedit-field-id" attribute on the div.field-item to see if we'd get lucky, but it didn't work. It seems like Quick Edit really needs the div wrapping the entire field. Fortunately it doesn't need the div.field-items wrapper. Based on this test I would expect Quick Edit to not work with the markup in the current issue summary, and included in patch #231, for single value fields:

<div class="field field--[modifiers] field__item">
 foo
</div>
mortendk’s picture

Yes ive been at this issue for a couple of years & some of the discussions have been beaten to death multiple times.
and Yes we need the outer div around the field, and thats actually an element we really wants for oocss & bem and all that ;)

<div class="field-item field-item--foo field-item--bar field-item--baz" attributes-field>....<div>

the reason were going with div's out of the box is that there's no way in hell were taking on the bikeshes again of h3 vs a span vs a div vs a whatever (that can be done in a follow up, but dosnt really matters as long as people can fix their markup emself)

We will try to keep this as agnostic as possible and all see if we can kill the divitis & the class soup as well (a trillion divs will scream when the hammer drops)

css class merging
on top of my head im pretty sure we can make all the css work directly in the template, and merge stuff there - Cause we have agreed upon we dont care if its "ugly" or hard to read in the templates, as long as we can find it, so in the case of modules wanting to add in classes if we grap them where we do the {% set class.... that could work out.

I must blank admit that i have no idae of how many real world scenarious we have where rdfa actual needs to be merged, or if were just panicking over changing something thats actually not used by anybody ? ;)

Anyways we have the following attributes that needs to be merged:

{# attributes-outer-wrapper #}
<div{{ attributes.addClass(classes) }}>
  {% if not label_hidden %}
{# attributes-title #}
    <div{{ title_attributes.addClass(title_classes) }}>{{ label }}</div>
  {% endif %}
  {# attributes-fields-wrapper #}
  <div{{ content_attributes.addClass('field-items') }}>
    {% for item in items %}
      {# attributes-field #}
      <div{{ item.attributes.addClass('field-item') }}>{{ item.content }}</div>
    {% endfor %}
  </div>
</div>

so we got these 4 wonderfull attributes that needs a bit of love / hammer:

attributes-outer-wrapper: {{ attributes }}
attributes-title: {{ title_attributes }}
fields-wrapper: {{ content_attributes }} 
item attribute: {{ item.attributes }}

Multiple fields are kinda done magic / trivial to fix
* Multiple fields with label shown (or invisible) we print it all as it is today - so Thats done (hurray)
* Multiple fields with No label we dont the title attributes, so Thats done (hurray) - we just need a patch that does that in the tempate & fixes the test.

single fields: - is where its at.
* single field with label: merge attributes-outer-wrapper: {{ attributes }} with fields-wrapper: {{ content_attributes }}
* single field with no label: merge attributes-outer-wrapper: {{ attributes }} with fields-wrapper: {{ content_attributes }} with {{ item.attributes }}

did i miss anything ?

mortendk’s picture

Heres another take on the template for fields

If we test on the attributes if they exist {% foo.storage is not empty %}
we can sneak our way around the pesky merging of attributes & the headaaches that cause, kill the divitis - unless theres something im not getting ?

This was done for testing purporses with a li tag instead of vaniall divs - its a POC ;)

{%
  set classes = [
    'field',
    'field--name-' ~ field_name|clean_class,
    'field--type-' ~ field_type|clean_class,
    'field--label-' ~ label_display,
  ]
%}
{%
  set title_classes = [
    'field__label',
    label_display == 'visually_hidden' ? 'visually-hidden',
  ]
%}

<div{{ attributes.addClass(classes) }}>
  {# label #}
  {% if not label_hidden %}
    <h2{{ title_attributes.addClass(title_classes) }}>{{ label }}:</h2>
  {% endif %}
  {# /label #}

  {# items add a wrappere if theres multiple of added attributes. #}
  {% if (items|length > 1 or content_attributes.storage is not empty) %}
    <ul{{ content_attributes.addClass('field__items') }}>
  {% endif %}

  {# Item. #}
  {% for item in items %}
    {% if items|length > 1 %}
    {# Multiple items. #}
      {% if (item.attributes.storage is not empty and not label_hidden) %}
        <li{{ item.attributes.addClass('field__item') }}>{{ item.content }}</li>
      {% else %}
        <li class="field__item">{{ item.content }}</li>
      {% endif %}
    {% else %}
    {# Single item. #}
      {% if (item.attributes.storage is not empty or not label_hidden) %}
        <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
      {% else %}
        {{ item.content }}
      {% endif %}
    {% endif %}
  {% endfor %}
  {# /print item #}

  {% if (items|length > 1 or content_attributes.storage is not empty) %}
    </ul>
  {% endif %}

</div>


ShaunDychko’s picture

Assigned: Unassigned » ShaunDychko
ShaunDychko’s picture

FileSize
36.45 KB
46.21 KB
2.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,663 pass(es), 3 fail(s), and 0 exception(s). View

@mortendk, testing for attributes is something I tried in the patch on #252, but as @yched (and you!) pointed out in #253 and #254, the markup needs to be predictable. Predictable markup is a comment appearing many times throughout this thread.

Further to the point about predictable markup, @scor mentions in #65 that CSS selectors could break if markup changes suddenly as a result of 2 vs 1 field item, or as a result of a div wrapping a field item, but changing suddenly to an "li" if another item is added. @dman explains nicely in #71 why reliable markup is such an important issue, and @joelpittet agrees in #72.

Attached is a patch that's simple and reduces divitis for fields configured to allow only 1 item (1 is the field "cardinality"). It avoids the need for merging attributes, and results in rock-solid reliable markup. Changing markup based on field configuration is reliable since it's a setting that can't be changed after the field has any data, and it will be the same for all instances of the field (ie. this setting can't be changed for each bundle to which the field is attached). Since the property mentioned by @mdrummond in #19 $variables['#cardinality_multiple'] seems to no longer exist, this patch has to pull in the fieldStorageDefinition.

The patch removes the variable $content_attributes, and the documentation of it in template_preprocess_field and in field.html.twig since it's unused. It shouldn't be mentioned in the documentation here since the div.field-items wrapper will often (usually!) not be printed.

In addition to the merging problem, this patch avoids issue with RDFa, and also creates a template that's simple to understand and modify.

Having a simple template is important. There are many comments in this issue critiquing the complexity of the templates being developed before #252. As mentioned in the parent issue #2483319: [META] Remove unnecessary markup from core templates, a.k.a. divitis , "The main goal of this issue is to improve themer experience."

I have intentionally not changed any classes in this patch since there is already an issue for that at #2214249: Fields classes - what do we actually need & want in Drupal8 twig. Since this issue is 261 comments long I'm trying to tackle only a smaller part of it in an effort to close one topic and move other topics (such as class names) to other issues.

There was no need to change any tests with this patch since the EntityReferenceFormatterTest created a field configured for unlimited values, in which case markup hasn't changed at all. Divitis is reduced as much as possible without breaking anything for fields configured to have only 1 item.

Attached are screenshots showing the results.

Field with only 1 value allowed. Div.field-items removed.

1 allowed value.

Field with unlimited possible values. One item entered. Div.field-items is present resulting in reliable markup regardless of number of items actually entered.

unlimited cardinality

ShaunDychko’s picture

ShaunDychko’s picture

Assigned: ShaunDychko » Unassigned
Status: Needs work » Needs review
ShaunDychko’s picture

Status: Needs work » Needs review
FileSize
0 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,681 pass(es). View

Fixed tests.

ShaunDychko’s picture

ShaunDychko’s picture

Status: Needs review » Needs work
ShaunDychko’s picture

Status: Needs work » Needs review
FileSize
3.96 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,664 pass(es), 2 fail(s), and 0 exception(s). View

Instead of testing for an object of a certain class, check whether a class method exists instead since the object type can vary.

dman’s picture

Identifying the .field-items wrapper as being the actual div that is truly redundant for single values, and killing *that* - looks like the sane thing to do.

It strips things down, AND means that if a themer were targeting that middle wrapper for a reason, that reason should have been 'because there are multiples'. The rest of the time, the targets should have been the outer and/or the inner.

ShaunDychko’s picture

Assigned: Unassigned » ShaunDychko

Need to fix tests and remove a couple debug lines.

ShaunDychko’s picture

Assigned: ShaunDychko » Unassigned
Status: Needs work » Needs review
FileSize
3.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,678 pass(es). View

Tests pass locally... let's ask the Drupal bot. Also removed a couple of debug lines.

mortendk’s picture

Issue summary: View changes
FileSize
37.53 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,667 pass(es), 5 fail(s), and 0 exception(s). View
275.2 KB

I came up with another solution, that instead of going with 4 different renderings - We have to test if am attribute have anything in it, if it do, then dont it also expect to be in the same place in the dom (ex: the "items wrapper" that we have now") so we need more versions, or we instead make a bunch of test of each part of a fields potential markup.
The field template got a bit more complex at at first, so I added a few preprocess variables instead that makes easier to read, and keeps the number of lines of the template down.

view-source:

the holy grail of 1 div for a single item is here
& yes quick edit still works

field.html.twig

{% if multiple_hiddenlabel_no_content_attributes %}
<div{{ attributes.addClass(classes).addClass('field__items') }}>
{% elseif multiple_visiblelabel_content_attributes %}
<div{{ attributes.addClass(classes) }}>
{% endif %}

  {% if not label_hidden %}
    <div{{ title_attributes.addClass(title_classes) }}>{{ label }}:</div>
  {% endif %}

  {% if multiple_items_wrapper %}
    <div{{ content_attributes.addClass('field__items') }}>
  {% endif %}

  {# Item #}
  {% for item in items %}
    {% if (multiple_fields or not label_hidden or item.attributes.storage is not empty ) %}
      <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
    {% else %}
      <div{{ attributes.addClass(classes).addClass('field__item') }}>{{ item.content }}</div>
    {% endif %}
  {% endfor %}
  {# /Item #}

  {% if multiple_items_wrapper %}
    </div>
  {% endif %}

{% if (multiple_hiddenlabel_no_content_attributes or multiple_visiblelabel_content_attributes) %}
</div>
{% endif %}

theme.inc

function template_preprocess_field(&$variables, $hook) {
....
// Do the field contains multiple items.
  if ( count($element['#items']) > 1)  {
    $variables['multiple_fields'] = TRUE;
  }
  if ( count($element['#items']) > 1 and $element['#label_display'] != 'hidden' ) {
    $variables['multiple_items_wrapper'] = TRUE;
  }
  // Check if theres multiple fields, the label is hidden and content_attributes is empty.
  if ( count($element['#items']) > 1 and $element['#label_display'] == 'hidden' and empty($variables['content_attributes']) ) {
    $variables['multiple_hiddenlabel_no_content_attributes'] = TRUE;
  }
  // Check if theres multiple fields or a visible label or content_attributes is not empty.
  if ( count($element['#items']) > 1 or $element['#label_display'] != 'hidden' or !empty($variables['content_attributes']) ) {
    $variables['multiple_visiblelabel_content_attributes'] = TRUE;
  }
....

divits is to have divs thats not needed, so theres no way out of kill 'em all, unless they have a real purpose ;)

mortendk’s picture

Having a simple template is important

Let me just make it clear that the one request we have gotten from every themer is that its not important, its important that the theme can change the markup to whats needed, if the trade of is that a template gets "complicated" then its worth it. We have had this discussion when we worked on the menu template.

Lets top thinking that themers dont understand complexety! its a strange and damanging developer myth in the drupal world. Just cause "themers" work with in the space where graphics & code interact - isnt the same as we dont understand complex issues, else I have 10 browsers and 4859305983798573 screen sizes and kinda documented css & markup to "understand".

only removing the field__items wrapper dont solve the issue when theres a single item with no label - which is the goal :)

ShaunDychko’s picture

Issue summary: View changes

just bookmarking Option #2 in the issue summary.

ShaunDychko’s picture

Issue summary: View changes
mortendk’s picture

FileSize
38.11 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,654 pass(es), 3 fail(s), and 0 exception(s). View

@ShaunDychko I dont understand the purpose of option 2 - please look at the template in #274 its 30 lines :)

This patch dont try to merge attributes together instead test if theres data in the attributes, and by that desides whats neede to be printed out for a field

new patch should have the 3 issues left - looks like its whitespace ?
anyways sending it to test, with all the classname fixings in bartik as well to follow the css naming standard ... so its growing kinda big

mortendk’s picture

Status: Needs work » Needs review
ShaunDychko’s picture

Status: Needs work » Needs review
FileSize
30.07 KB
19.58 KB
25.62 KB
30.08 KB

Hi @mortenkd, thanks for inviting more input. We're totally on the same page that the markup needs to be leaner, which is why I'm trying to give this issue some attention.

Complexity of the template is not the main issue. With the approach taken in #279, where the existence of attributes is tested to determine whether to print a tag, the markup is unpredictable and will change suddenly and unexpectedly. I applied #279 to demonstrate. I had to also apply the patch in #2537288: Content=Value attribute should be printed for numeric fields with a Prefix or Suffix since that was the easiest way I could think of for creating attributes on div.field-item. I created a numeric integer field, and attached is the markup with a prefix, and another without a prefix for the number (that's a setting in the field config). The prefix causes a "content=value" attribute to be created for div.field-item. The label is hidden.

Without Prefix

<div data-quickedit-field-id="node/1/field_test_multiple_numbers/en/full" class="field field--name-field-test-multiple-numbers field--type-integer field--label-hidden field__item quickedit-field">2</div>

no attribute

With Prefix

<div content="2" class="field__item">ThePrefix2</div>

with attribute
There's no typo there, and I'm as surprised as you probably are that all the wrapping div's disappeared when the prefix was added. I would imagine this particular problem could be solved in the next iteration of #279, but there's more...

Here's a less dramatic, but still problematic, example:
Create a plain text field with an unlimited number of allowed values.

Only one value entered

<div data-quickedit-field-id="node/1/field_some_/en/full" class="field field--name-field-some- field--type-string field--label-hidden field__item quickedit-field">first item</div>

one item

Two values entered

<div data-quickedit-field-id="node/1/field_some_/en/full" class="field field--name-field-some- field--type-string field--label-hidden field__items quickedit-field">
  <div class="field__item">first item</div>
  <div class="field__item">second item</div>
</div>

two items

The markup has suddenly changed just because an editor added a second value to the field. If the CSS was originally created for the "One value entered" markup, then possibly some styles won't apply properly when the markup changes. I could imagine that if the selector div.field--name-field-some- was used originally, maybe there will be a style that won't inherit to the div.field--name-field-some- > div.field__item that appears after adding the second value?

Testing attribute values is tempting, and I also gave it a go in the patch I made in #252, but I then became convinced that it isn't a good approach due to the unpredictable markup it creates.

There are other ways in which attributes will suddenly appear, such as RDFa, or maybe a module is installed that creates custom data-attributes on field items to enable some javascript functionality... it's really hard to imagine all the possibilities, but undoubtedly there are many ways in which attributes will appear, or not, for field items, and each appearance/disappearance of field item attributes will make significant changes to the markup. This is why the approach in #279 is unfortunately not the way to go.

Having field markup output without any wrappers when labels are hidden would be fantastic. This is the kind of "as lean as can be" markup you're shooting for, and it's obvious you feel strongly about it given the enormous work you've put into exploring several different approaches. First there was the "merge attributes" approach, then the "test attributes" approach. This work you've done is really important. Had I found this issue in the beginning, I probably would have tried some of the same things. It's only through seeing the challenges the other approaches encountered that I was convinced to settle for a small win, rather than the big win you're aiming for, by just removing the div.field-items wrapper for single value fields in the patch of #273.

mortendk’s picture

@ ShaunDychko
thanx a ton for jumping on on this & help out throwing the field aroung (yes im hellbent to fix this)
The reason i went for the "lets test if theres any data in the attributes" is that it makes a whole lot of sense - thanx for pointing me that way, my brain was to focused on making it clean compared to the prototypes we did earlier.

Markup change is expected :)

the markup is unpredictable and will change suddenly and unexpectedly

Nope the markup will change if there's a need for it to change, its by design - its not unexpected if a field change its markup if more than than one field, after a user have added more data - Its expected that it changes and with drupal8 using bem this should not be an issue:

field{ border-bottom: 1px solid hotpink; margin: 1rem 0;}
field__item{ display: inline-block; background: pink}
field__label{ display: inline-block }
field__items{ display: inline-block; border-left:3px solid black }

change cause of a module suddenly adds in data or theres more than one field item

.field--name-foo{ border-top:3px dashed gray }
.field--name-foo .field__label{ border: none; float: left}
.field--name-foo .field__items{ border: none; float: left}

we did a bunch of test on the css way back, when we started this issue http://codepen.io/mortendk/pen/rvxhw

Remember that the themer knows that this is happening, the markup patterns is predictable, we change the markup based on what's needed to describe the data & add the classes. An importent thinh to remember is that we don't want to fix 100% of every usecases, we want to fix the 80%. its one of the core values that were building Drupal8 theming on.

So to sum it up:
Themers are ok with a complex template.
Themers are ok with markup changes, when there's a reason for it.
Themers are ok with having to make changes, when its needed.
We dont hit 100% of all use cases, thats fine - thats expected.

Divitis is the bane of Drupal - its the single biggest wtf & yes i would dance in the moonshine & sacrifice blood to get it outta drupal once & for all.

the Prefix is a wtf, looks like my test are missing something - patches are more than welcome ;)
Were so close, its a test on markup that have whitespace issues and the prefix - you can almost smell the victory.

rockitdev’s picture

I like the apporach mortendk is proposing.

I don't think you can easily account for 100% of all use cases (and you shouldn't try to). Markup should be simple as possible, and if site builders or themers want to add more, then it should be simple for them to do it through the templating system. I don't like the approach of throwing the kitchen sink at everything.

From my experience, themers or developers who are concerned with data attributes or rdfa on a div aren't typically people who are scared of getting into a bit of logic.

mdrummond’s picture

Assigned: Unassigned » mdrummond

I'm going to give this a go based on some of the earlier approaches that seemed close to working.

mdrummond’s picture

Assigned: mdrummond » Unassigned
FileSize
37.68 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,827 pass(es), 6 fail(s), and 0 exception(s). View
8.16 KB

Laurii had a patch in #231 that was actually green and did nested array deep merging and also setting a multiple variable based on the field definition. While that patch had some other issues, I brought some of those concepts into mortendk's patch in #279, preserving the markup patterns that had been agreed to a long while back.

This should simplify the markup while maintaining consistency.

- If a field has a label, then it will have an outer wrapper.
- If a field has the potential for multiple values, it will have a field-items wrapper around field-item.

So if you want styles to stay consistent, no matter what:
- Use the straight-up .field classes to maintain the relationship of this field with whatever is around it
- Use .field-item to maintain relationships between multiple values for the same field
- Use .field-items for anything needed for the field items as a whole when there are multiple potential field values

There will always be a .field-item class. Always. So if you need to maintain a bottom margin below a field, something like this could be handy:

.field.field__item,
.field .field__item:last-child {
margin-bottom: 1em;
}

I think this template is fairly straightforward and should create markup that is doable to theme against consistently. If Testbot gives the thumb up, then we'll see what's next.

davidhernandez’s picture

+++ b/core/includes/theme.inc
@@ -1498,8 +1498,9 @@ function template_preprocess_field(&$variables, $hook) {
-  $variables['field_name'] = $element['#field_name'];
-  $variables['field_type'] = $element['#field_type'];
+  $variables['field_name'] = strtr($element['#field_name'], '_', '-');
+  $variables['field_type'] = strtr($element['#field_type'], '_', '-');
+  $variables['field_label'] = strtr($element['#label_display'], '_', '-');

See my previous comments (#239). Why is this creating class names in code, especially when clean_class is then used on them in the templates? There seems to be some legacyness going on here.

+++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -128,9 +128,9 @@ public function form(FieldItemListInterface $items, array &$form, FormStateInter
         'class' => array(
-          'field-type-' . Html::getClass($this->fieldDefinition->getType()),
-          'field-name-' . Html::getClass($field_name),
-          'field-widget-' . Html::getClass($this->getPluginId()),
+          'field--type-' . Html::getClass($this->fieldDefinition->getType()),
+          'field--name-' . Html::getClass($field_name),
+          'field--widget-' . Html::getClass($this->getPluginId()),

Same

mdrummond’s picture

Status: Needs work » Needs review
FileSize
38.57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,816 pass(es), 7 fail(s), and 0 exception(s). View
4.41 KB

Trying to fix problems causing test errors.

I also got rid of the variable cleanup that was already being done in the template. I did not see field_label being used, so I eliminated that variable.

The widget classes seemed different. That looks like it is maybe on the admin side? We don't put in a widget class elsewhere so that might be necessary? Not sure.

The entity reference formatter error seems to be a spacing bug that I can't seem to fix.

The standard profile errors, I don't know. Maybe something to do with the merging of attributes since it is rdf related. Could use help debugging. Maybe Laurii could help with that?

mdrummond’s picture

Status: Needs work » Needs review
FileSize
38.97 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,794 pass(es), 33 fail(s), and 0 exception(s). View
4.58 KB

This should fix the spacing test errors by using spaceless. Not my favorite solution, but the specificity for space control required in the entity reference formatter test is extraordinarily picky. On a production site, space will be zapped away anyhow, and with inspect element you will still see proper nesting. So I think spaceless seems like a good solution here.

This may also fix the attribute merging causing the other test failures. That test won't run on my local, though, so we'll see.

mdrummond’s picture

Always exciting when you fail in new and different ways.

Now that spaceless is in place, my guess is there are other assertions that need space removed from the expected markup.

The attribute merging is still a problem unfortunately.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
39.45 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,805 pass(es), 28 fail(s), and 0 exception(s). View
4.02 KB

Local tests aren't working very well, so trying to narrow down if the problem was with spaceless or with switching around the attributes that caused so many new failures. I suspect it's the attributes, but Testbot will let us know.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
39.42 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,831 pass(es), 13 fail(s), and 0 exception(s). View
499 bytes

Still a bunch of errors. Let's see what happens when I remove spaceless.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
38.58 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,825 pass(es), 6 fail(s), and 0 exception(s). View
3.7 KB

This fixes the errors clearly related to spacing.

The issues with the standard profile and comments field I am somewhat stumped on. If anybody has ideas, I could use them.

mdrummond’s picture

Well at least just down to the standard profile bugs now. Hard to see from the test what exactly is going wrong. Doing debug on that $graph variable caused pretty long hang times when I tried it.

The last time that bug was fixed was in #231, but the way it was fixed was to not merge in the item attributes with the main attributes and content attributes. That was possible because there was still a field__item div inside of the field div. Losing that would be disappointing, because it would mean an extra div when it isn't needed.

We are so close. This has to be fixable.

andypost’s picture

mdrummond’s picture

Status: Needs work » Needs review
FileSize
39.11 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,907 pass(es), 6 fail(s), and 0 exception(s). View
5.56 KB

ShaunDychko had posited that we don't need content_attributes in field templates. So this is an experiment to test if it works to remove that and thus remove the need for some of the attribute merging. Testbot will tell us yay or nay.

mdrummond’s picture

My takeaway from this experiment is that content_attributes merging is not the issue. My guess is that merging item attributes with attributes is the problem. Might be worth manually testing this to see if quickedit works when field__item is merged with field. There was mention of that earlier, and if it doesn't work, then we'll have to make decisions if that level of div killing is actually possible or not.

mdrummond’s picture

Good news. Quick edit still works when field__item is not nested under field.

So that means we just need to figure out what the heck is going on with the standard profile that is so problematic.

scor’s picture

I'll take a look at the std profile test this afternoon.

mdrummond’s picture

Thanks so much scor!

scor’s picture

Status: Needs work » Needs review
FileSize
40.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,296 pass(es). View
1.42 KB

Wow, I gotta thank @mortendk and personally buy him a beer in Barcelona for starting this whole field divitis removal initiative. Here is why: not only is this patch going to clean the markup in general, but I just realized that it has also allowed me to improve the RDFa markup for node author! (and fix the tests at the same time). The attached patch passes the standard profile tests on my local machine, let's confirm this with the testbot.

I can also confirm that we're not using content_attributes in core. It was added in Drupal 7 to be consistent with title_attribute (which we do use for RDFa), but if we wanted to remove content_attribute, it's fine by me. In fact this was pointed out a long while back by @effulgentsia but never acted upon.

emma.maria’s picture

Issue tags: +Needs manual testing
davidhernandez’s picture

+++ b/core/modules/rdf/src/Tests/StandardProfileTest.php
@@ -107,6 +107,10 @@ class StandardProfileTest extends WebTestBase {
+    // Use Classy theme for testing markup output.
+    \Drupal::service('theme_handler')->install(['classy']);
+    $this->config('system.theme')->set('default', 'classy')->save();

This shouldn't be needed. It is a webtest and should already be using Classy.

mortendk’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
332.5 KB

by manual testing the markup, it seems that {% if multiple %} is never kicking in, so the .field__items wrapper never get to be printed out

Did i miss something or is it just not presented ?

mdrummond’s picture

Status: Needs work » Needs review
FileSize
40.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,338 pass(es). View
734 bytes

You're right, mortendk. The checking of isMultiple is never happening.

I'm fixing a typo in this patch, but from what I can tell, getFieldDefinition is not callable on $element['items']. We need to figure out where we can use getFieldDefinition to move forward. Anybody know?

ShaunDychko’s picture

In case it's helpful, I used

+  if (method_exists($element['#items'], 'getFieldDefinition')) {
+    $fieldStorageDefinition = $element['#items']->getFieldDefinition()
+      ->getFieldStorageDefinition();
+    $variables['cardinality'] = $fieldStorageDefinition->getCardinality();
+  }

in the patch at #273. If the cardinality is not 1, then it's configured for multiple.

mdrummond’s picture

FileSize
40.57 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,320 pass(es). View
1023 bytes

Thanks for that, Shaun. That was the key, as we were not using method_exists, and we also needed getFieldStorageDefinition to make this work.

This patch will fix the markup, but my guess is it will trigger some new test errors, since field__items will show up in places it was not showing up before.

mdrummond’s picture

No new test errors. Nice. Looks like we're back to manual testing.

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll
FileSize
693.06 KB

I have found some visual regressions in Bartik of the default Tags field supplied in the Article content type.
The colon no longer prints and there is now more space being added below it.
 

 
It looks like we have taken too many classes out of field--node--field-tags.html.twig file in Bartik maybe, I will take a look. This regression has also made me realise that we are styling one specific individual instance of a taxonomy term field called Tags and not every Taxonomy term field the same way. Which I also want to fix either in this issue or a follow up.

Anywho the latest patch needs a reroll because of #2395853: Split system.module.css and system.theme.css files into SMACSS style components being committed.

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
40.18 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,468 pass(es). View
mdrummond’s picture

FileSize
40.27 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,471 pass(es), 1 fail(s), and 0 exception(s). View
1.79 KB

These lines were removed from field.theme.css:

.field--label-inline .field__label::after {
  content: ':';
}

Instead a : was added after the field label within the template. So instead of : showing up only for inline fields, it now always shows up. The node tags field template was not updated with this change in Bartik, hence the discrepancy. But a larger question is if there are any thoughts about this change to show a : on labels that are not inline. That is a change.

I can't see anywhere in this thread where that change was actually discussed. So I'm reverting back to only adding in the colon for inline labels. That seems a lot more in line with expectations (pun partially intended).

The extra space was due to the addition of vertical-align:top to the field__label. It looks like that was added from the patch in #147 after a comment about some missing vertical space. There are also discrepancies in the line heights, as there are differences in the font sizes of the tag container versus the label and items. One of the gazillion reasons I would rather use rems than ems any day of the week.

Anyhow, after some testing, the only reliable fix I found for that was reverting the change from float left to display inline-block. Moving back to float left lets the container have the same height as the label and items.

Hopefully this gets us close. Maybe we need to test to see if the vertical problem that #147 was supposed to solve is still a problem? I wasn't clear what the issue was.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
40.27 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,458 pass(es). View
754 bytes

This should fix the test.

emma.maria’s picture

Thanks @mdrummond for fixing those things. Yes I want to overhaul Bartik with rems also. I will do some more manual tests on the patch in #320 this evening.

yched’s picture

+++ b/core/includes/theme.inc
@@ -1508,11 +1506,24 @@ function template_preprocess_field(&$variables, $hook) {
+  if (isset($element['#items']) && method_exists($element['#items'], 'getFieldDefinition')) {
...
+    if (method_exists($fieldDefinition, 'getFieldStorageDefinition')) {

Curious why those method_exists() checks are needed.

- The content of $element['#items'] should by contract be a FieldItemListInterface object, with a getFieldDefinition() method.
- Likewise, ->getFieldDefinition() should always return a FieldDefinitionInterface, on which you can always call getFieldStorageDefinition()

The fact that we need to safeguard those method calls feels like it's adding needless uncertainty, or is a smokesign that something's off somewhere upstream.

- Is that to account for the possibility of NULL ? Then "if ($foo = $element['#items']->bar())" checks would be more suited.
- At the very worst, if there *is* unpredictability (actual cases where the result might be non-NULL objects that don't have the methods), then we should use "instanceof SomeInterface" checks rather than method_exists. But, again, that sounds very unlikely.

Can we remove those checks and see what breaks ?

mdrummond’s picture

FileSize
40.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,473 pass(es), 1 fail(s), and 0 exception(s). View
872 bytes

Used the reduced check that yched suggested and tested it thoroughly. Seems to work fine. Let's see what Testbot says.

mdrummond’s picture

yched, looks like QuickEdit is having problems without those checks. Specifically lines 412 and 572.

412 is testing a pseudo field, quickedit_test_pseudo_field, as defined in quickedit_test.module.

572 is testing a field image.

Any insights on what might be different about those fields that would cause a problem here?

emma.maria’s picture

I ran some more visual regression tests using the patch in #323.

Now everything looks perfect in Bartik!!

As long as nothing to do with Bartik changes in future iterations of patches I sign this off.

Thanks for all of the hard work in this issue!! Thank you @mdrummond for taking the time to fix Bartik in #318.

Test results attached.

yched’s picture

re: fails - wow, it's quickedit_test_entity_view_alter() that does unholy things :-)

It adds a bespoke "rendered field" to the render array, that is hand-made rather than the actual output of an actual formatter.
It uses '#theme' => 'field' and thus triggers template_preprocess_field(), but its content is not strictly compatible with what a real formatter would output (see FormatterBase::view()) - notably its '#items' entry is an array of values instead of an actual "field items" object implementing FieldItemListInterface. That cannot fly, IMO it's not reasonable to expect template_preprocess_field() to handle mixed input.

Respective code snippets :

function quickedit_test_entity_view_alter(&$build, EntityInterface $entity, EntityViewDisplayInterface $display) {
  if ($entity->getEntityTypeId() == 'node' && $entity->bundle() == 'article') {
    $build['pseudo'] = array(
      '#theme' => 'field',
      // ...
      '#items' => array(
        0 => array(
          'value' => 'pseudo field',
        ),
      ),
      0 => array(
        '#markup' => 'pseudo field',
      ),
    );
  }
}
@@ -1508,11 +1506,21 @@ function template_preprocess_field(&$variables, $hook) {
+  $variables['multiple'] = FALSE;
+  if ($itemsDefinition = $element['#items']->getFieldDefinition()) {
+    $variables['multiple'] = $itemsDefinition->getFieldStorageDefinition()->isMultiple();
+  }

Not sure why quickedit_test_entity_view_alter() uses a fake entry rather than an actual formatter ? It was added in #2057973: Don't add Edit module's data- attributes on pseudo fields (as used e.g. by Display Suite), maybe @aspilicious or @Wim could chime in ? I'll ping over there.

yched’s picture

If we want to unblock this issue here, I guess we could temporarily do :

$variables['multiple'] = is_object($element['#items']) ? $element['#items']->getFieldDefinition()->getFieldStorageDefinition()->isMultiple() : FALSE;

with a @todo on a followup issue to remove the conditional after fixing quickedit_test_entity_view_alter(). $element['#items'] should by contract be a FieldItemListInterface, template_preprocess_field() (or any other custom field preprocessors or templates trying to access the field definition) shouldn't have to safeguard for tests that take shortcuts :-)

swentel’s picture

Hmmm, yeah, I guess you can kill me for this because I figured four years ago that this was a good idea to render those 'pseudo' fields (I don't necessarily think it's such a bad idea, but oh well).

So yeah, I agree with yched's comment in #328 - create a follow up to unblock this issue. Then we can figure out in that one how contrib should (or should not) deal with this.

yched’s picture

Opened #2550225: quickedit_test_entity_view_alter() creates a non compliant field render array - meanwhile, yeah let's do #328 with a comment pointing to quickedit_test_entity_view_alter() and a @todo on the issue.

[edit: crosspost, wink at @swentel]

mdrummond’s picture

yched: Thanks for figuring that out! Just a heads up that there was also a test error with an image as well as QuickEdit. So there might be something going on with the image field as well.

I'll get an updated patch posted.

mdrummond’s picture

First off we need a reroll as the previous patch wasn't applying.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
37.58 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,207 pass(es). View
920 bytes

Here's an updated patch with the code and comments suggested in #330.

mortendk’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Twig, -Needs screenshots, -Needs manual testing
FileSize
248.77 KB
203.44 KB
163.91 KB
161.4 KB

Manual tested this looks good (we can allways later discuss if we wanna cut down further on classes etc but thats another issue)

view-souce classy:

Bartik screens combined

Bartik before :

Bartik after the great div kill:

pushing to rtbc -we have the followups mention in #330 #2550225: quickedit_test_entity_view_alter() creates a non compliant field render array

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. Bartik's field.css will be broken by this change. Lot's to update there...
  2. .node__meta .field-name-field-user-picture img {
      float: left; /* LTR */
      margin: 1px 20px 0 0; /* LTR */
    }
    [dir="rtl"] .node__meta .field-name-field-user-picture img {
    

    in Bartik's node.css will be broken.

  3. The new rdf_preprocess_field() can handle the comment uid too, no? See rdf_preprocess_comment()
mdrummond’s picture

Status: Needs work » Needs review
FileSize
41.9 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,176 pass(es). View
4.32 KB

This should fix the CSS based on the markup changes. Nice catch.

I'm not clear on what needs to change based on point 3.

alexpott’s picture

+++ b/core/modules/rdf/rdf.module
@@ -271,6 +271,18 @@ function rdf_preprocess_html(&$variables) {
 /**
+ * Implements hook_preprocess_HOOK() for field templates.
+ */
+function rdf_preprocess_field(&$variables) {
+  // Swap the regular field property attribute and use rel instead so that it
+  // plays well with the RDFa markup generated for the username.
+  if (!empty($variables['attributes']['property']) && $variables['field_type'] == 'entity_reference' && $variables['field_name'] == 'uid') {
+    $variables['attributes']['rel'] = $variables['attributes']['property'];
+    unset($variables['attributes']['property']);
+  }
+}
+

@@ -297,12 +309,6 @@ function rdf_preprocess_node(&$variables) {
-  // Adds RDFa markup for the relation between the node and its author.
-  $author_mapping = $mapping->getPreparedFieldMapping('uid');
-  if (!empty($author_mapping['properties']) && $variables['display_submitted']) {
-    $variables['author_attributes']['rel'] = $author_mapping['properties'];
-  }

So rdf_preprocess_field() applies to all entity types including node. But should we limit this to only nodes? I don't know the answer to that... perhaps @scor can enlighten us?

mdrummond’s picture

One thing I discovered in making the Bartik updates is that the field type for tags and other taxonomy terms is type-entity-reference and not type-taxonomy-term-reference. So none of those styles were applying, which is why a visual comparison of Bartik in HEAD looked the same as Bartik in this patch.

So that means that now if you compare this with Bartik in HEAD, there *will* be a discrepancy. But presumably that's because there was a Bartik regression that wasn't caught before, and now it should look like how Bartik should look.

scor’s picture

According to the markup, and confirmed by my debugger, we're not passing through rdf_preprocess_field() with the uid field for comments because the author is not rendered as a field in comments. Not sure if there is a plan to migrate uid as a field for comments, but that would make sense.

mdrummond’s picture

Would it make sense to do that part in a follow-up?

mortendk’s picture

Issue summary: View changes
FileSize
1.69 MB
1.69 MB
367.34 KB
367.34 KB
627.96 KB
627.96 KB
212.38 KB
204.27 KB
271.67 KB

yes it would make a lot of sense to do followups, were not talking about killing divitis (in #339) but changing rendering on elements, it seems like to be outta scope, unless im missing something.

Redid the Visual regression test on bartik -an article, a node with all fields & a frontpage listing.

Screenshots compared

scor’s picture

See #2227503: Apply formatters and widgets to Comment base fields for the on-going work to make uid a field on comments. I can't think of a reason why we would not want to keep the current rdf_preprocess_field() so it works when we also have uid as a field in comment. If for some reason something is missing in the RDFa markup for comments, the RDF tests would catch it.

alexpott’s picture

@scor but it's not just about comments it's about the rendering of any entity type with a uid field.

mdrummond’s picture

My understanding of where we're at:

- @alexpott's concern is that previously the properties for the UID field were added to the rel attribute only for nodes, but now we're expanding that to all entities.

- @scor thinks that it's good that this will work if there is a UID field on a comment entity.

- @alexpott still wants to know if there will be an issue setting this rel attribute when there are UID fields on entities that are not nodes or comments.

Does that sound right?

If so, what I think we need is clarification on point three from @scor, right?

andypost’s picture

Entities must never checked for "uid" field!
There's \Drupal\user\EntityOwnerInterface that should be used with class_implements() on entity

lauriii’s picture

FileSize
41.92 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,908 pass(es). View

Simple reroll

lauriii’s picture

mdrummond’s picture

Laurii, I'm unclear what that issue is trying to do or how it relates to this one.

lauriii’s picture

FileSize
42.24 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,878 pass(es), 3 fail(s), and 314 exception(s). View
1.31 KB
42.49 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,919 pass(es), 2 fail(s), and 40 exception(s). View
544 bytes

Lets see if I can demonstrate this here

lauriii’s picture

Status: Needs work » Needs review
FileSize
42.48 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,938 pass(es). View
416 bytes

Sorry for a bad demo. This one might be better

scor’s picture

FileSize
42.35 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,974 pass(es). View
1.29 KB

We don't need rdf_preprocess_field__node__entity_reference(). Entity reference fields in nodes (and in general) work well on their own. In this patch, I've also improved some comments in rdf.module.

Edit: I believe this new approach should address @alexpott's comment #343

lauriii’s picture

Status: Needs work » Needs review

WTF for me it seems it has passed

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

Since the UID issue appears to have been addressed, moving this back to RTBC.

Pages