Visually Hidden & Inline field label setting have no effect [#2652094]

By default you can set field label visibility in: admin/structure/types/manage/items/display.
Themes based on Classy follow these settings and can display them as 'Above', 'Inline', 'Hidden' and 'Visually-Hidden'.
As Neato is not using Stable we are not passing on these classes in the template.
I can't figure out if this is an oversight or intentional in core but am wondering if Neato should add in this functionality itself.

Comments

Comment #1

18 January 2016 at 13:28

Lostandfound created an issue. See original summary.

Comment #2

serg2 CreditAttribution: serg2 commented 18 January 2016 at 13:30

Issue summary:

View changes

Comment #3

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 17:59

Issue summary:

View changes

Comment #4

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 18:00

I don't think using Stable matters. Since there are no field template twig overrides, shouldn't it be rendering with the field.twig.html file from the core system module like it had in previous versions of Drupal?

Comment #5

serg2 CreditAttribution: serg2 commented 20 January 2016 at 19:23

These are the field.html.twig from core/module/system/templates , Stable & Classy:

core/module/system/templates:

{% if label_hidden %}
  {% if multiple %}
    <div{{ attributes }}>
      {% for item in items %}
        <div{{ item.attributes }}>{{ item.content }}</div>
      {% endfor %}
    </div>
  {% else %}
    {% for item in items %}
      <div{{ attributes }}>{{ item.content }}</div>
    {% endfor %}
  {% endif %}
{% else %}
  <div{{ attributes }}>
    <div{{ title_attributes }}>{{ label }}</div>
    {% if multiple %}
      <div>
    {% endif %}
    {% for item in items %}
      <div{{ item.attributes }}>{{ item.content }}</div>
    {% endfor %}
    {% if multiple %}
      </div>
    {% endif %}
  </div>
{% endif %}

Stable:

{% if label_hidden %}
  {% if multiple %}
    <div{{ attributes }}>
      {% for item in items %}
        <div{{ item.attributes }}>{{ item.content }}</div>
      {% endfor %}
    </div>
  {% else %}
    {% for item in items %}
      <div{{ attributes }}>{{ item.content }}</div>
    {% endfor %}
  {% endif %}
{% else %}
  <div{{ attributes }}>
    <div{{ title_attributes }}>{{ label }}</div>
    {% if multiple %}
      <div>
    {% endif %}
    {% for item in items %}
      <div{{ item.attributes }}>{{ item.content }}</div>
    {% endfor %}
    {% if multiple %}
      </div>
    {% endif %}
  </div>
{% endif %}

Classy:

{%
  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',
  ]
%}

{% if label_hidden %}
  {% if multiple %}
    <div{{ attributes.addClass(classes, 'field__items') }}>
      {% for item in items %}
        <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
      {% endfor %}
    </div>
  {% else %}
    {% for item in items %}
      <div{{ attributes.addClass(classes, 'field__item') }}>{{ item.content }}</div>
    {% endfor %}
  {% endif %}
{% else %}
  <div{{ attributes.addClass(classes) }}>
    <div{{ title_attributes.addClass(title_classes) }}>{{ label }}</div>
    {% if multiple %}
      <div class="field__items">
    {% endif %}
    {% for item in items %}
      <div{{ item.attributes.addClass('field__item') }}>{{ item.content }}</div>
    {% endfor %}
    {% if multiple %}
      </div>
    {% endif %}
  </div>
{% endif %}

I could be misunderstanding this but it seems that loading up a site with Bartik will display 'properly' as it is based on Classy whereas any theme based on Stable won't have those classes/attributes added. My initial question was whether you thought Neato should have this feature by default but now I think it is more of a stable/core bug. Twig is still new to me so if I am getting this all wrong then let me know!

Comment #6

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 19:53

In Drupal 7, provided templates for render functions would use the default unless overridden (in a module or theme). It would seem to me that since I have not defined any field twig templates, the one in system module would (should?) be used. That was the behavior in Drupal 7. If you did not have a page.tpl,php file in your theme, the system would use the one in its templates folder.

Has this changed in Drupal 8? It would suck to have to supply all the templates in the system module folder. So, what classy and stable are doing are applying classes in the template - but I am guessing there are no attributes supplied by default. This was similar to what was reported here #2643090: Custom Twig Template not printing HTML or Classes. In fact, in the header of the file, it says:

 * To override output, copy the "field.html.twig" from the templates directory
 * to your theme's directory and customize it, just like customizing other
 * Drupal templates such as page.html.twig or node.html.twig.

Which implies that it should render with this file. I assume the change from before is that now Drupal doesn't force these classes on you via preprocess/render/theme functions in the core modules, instead, they are added by the theme twig files as evident from the classy/stable comparison.

I just looked at the Bootstrap theme, and they are supplying what looks like most of the templates (if not all) from system modules:

http://cgit.drupalcode.org/bootstrap/tree/templates

That is a lot of work to supply them back in my opinion.

Should we follow suit? How many template files would be required? I wasn't really anticipating that from Drupal 8.

=====

edit: upon looking at both the Classy and Stable theme, they do in fact have a lot of provided templates. It seems that is required now if your base theme isn't Classy or Stable... that kind of sucks.

Are we sure this is not a core bug?

Comment #7

serg2 CreditAttribution: serg2 commented 20 January 2016 at 20:00

It is worth noting that there is a lot of ongoing work at #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme removing templates from Core and putting them into Classy. But.... this whole area is a bit of a mess, "Stable" came into play very late so sometimes when reading through core issues they are referring to Classy which now means Stable. So, it might be a bug, might be by design.

Stable will be stable but is missing things which will could be added by Neato.

As I wasn't sure what was missing my solution was simply to clone all the templates from Classy and put them in my Neato sub-theme. Neato is still based Stable so it wont change but has all the classy features.

Comment #8

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 20:17

Neato isn't based on either theme. I didn't realize that going into Drupal 8, if you did not want to use Classy, you were on your own. But, by copying in all those templates from Classy/Stable into Neato, you overrode Neato and are basically using those themes + Neat SASS.

I hate to don a tinfoil hat, but it feels like this was done to correct the perception that Drupal is hard to theme and make you simply extend from one or the other. It just irks me to think that now you have to inject these classes directly in the twig template files or a preprocess file (but it looks like most people are setting it right in the Twig file).

I did not anticipate this happening... and if this is the new normal, I will eventually get used to it.

For phase 1 of the consensus banana, we moved all classes from preprocess into the templates.

That explains it.. no classes are coming from core, they have to be explicitly set by you or by the base theme you extend from. This also explains to me why a lot of contributed themes are extending Classy because they simply don't want to handle the added overhead and work of getting what you got by default in Drupal 7, which I can understand.

So... my initial thoughts on these here is, what is the minimum and most critical templates we can add to Neato? It also looks like we will be forced to copy the class assignments that others are doing just so the expected behavior that was exhibited in Drupal 7 (of having field classes, etc, applied).

Comment #9

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 20:35

If you want your theme to include Drupal's classes on your elements, define Classy as your base theme.

If you want your theme to have the minimum amount of classes, then do not use Classy as a base theme.

Welp, there you have it.

This creates a fairly large amount of work for any contrib theme as I see it who does not extend from Classy.

Now that I have thought about it a little bit, here is what it really means:

8.x for Neato works just like 7.x, but is missing a majority of classes out of the box
To regain these classes, add field.twig.html to your theme and use the set_classes ... attributes.addClass() method on the element to class it up

Comment #10

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 20:43

Apologies, you are right - Neato is extending Stable because there is no flag in the info file saying not to do that. That must be something new, I was not aware of that at all.

So, okay, Stable is being pulled in by default. That is the markup, there are just no classes.

The question is, how to explain and handle this for users.

Does this affect contributed modules as well? I am a little confused on these changes. If a module provides template(s) - is it expected for classes to be passed in a function or set right in the template file? If they don't gel, do you just override and exclude the classes in your new file?

Comment #11

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 21:01

Okay, wow, learned something new today. I totally overthought this because I've had four cups of coffee.

In neato.info.yml, I can add 'base theme: classy' and everything appears to be kosher. It inherits missing templates from Classy, which is better than nothing at all. I just tried this locally, and it worked.

I was always under the impression that you could only have one base theme.

Comment #12

serg2 CreditAttribution: serg2 commented 20 January 2016 at 21:10

Okay, that makes sense but not sure it is best. Classy will change regularly so, as you are extending from it, your theme may break.
It sort of comes down to what you want from this theme, how much you want to develop it etc.

Comment #13

kevinquillen CreditAttribution: kevinquillen commented 20 January 2016 at 21:14

Yeah, it is kind of like pick your own poison in that sense. If we copied all the Classy templates into Neato, then we have X files to maintain (if they change for some reason). Extending classy kind of gives us a little room to selectively override its templates and the time to do so.

Comment #14

ultimike

English

Florida, USA

CreditAttribution: ultimike at DrupalEasy commented 22 April 2016 at 14:38

I just updated from Neato 8.x-1.2 to 8.x-1.4, and my sub theme based on Neato needed a bit of work to get back in shape. It took me a few minutes to notice that base theme change from stable to classy.

For me, the switch caused some markup to change, which threw off a bunch of my CSS.

I'm not sure if the base theme switch came in 1.3 or 1.4, but it probably should be mentioned in the release notes, as it would have saved me some time.

thanks,
-mike

Comment #15

kevinquillen CreditAttribution: kevinquillen commented 22 April 2016 at 17:42

Sorry about that. At the time I did not realize that the default theme would be Stable if nothing is specified, while people were expecting classes to exist out of the box from Classy. So I changed the base theme to classy to inherit those templates instead, before realizing Classy also will inject a ton of style. I later learned that the best practice in this case is to copy all templates from classy to your theme to start a 'brand new' theme. Learning process of D8. Ideally, a future update will just copy the templates, not mark Classy as the base theme.