Umami Theme - follow-up - remove elements, use classes in CSS

1. I have created a theme template override for a link field, and any link generated by link filed will no have a class "link", it give us possibility to use .link class name instead of a link selector in CSS
2. I have implemented a hook_preprocess_menu_local_task() on theme level to mark local task menu links with special classes "tabs__link"
3. Updated appropriate CSS files

Mark it RTBC as it works. Thanks @hiway!

I have some thoughts, however. (We may create followup issues.)

We have this already (following Classy):
.tabs .tab {

A .tab (an li element) never will be found outside .tabs (a ul element) so it should be something like .tabs__item.

From the patch:
.tabs .tab__link {

One of the goals of styling (BEM) classes instead of HTML elements is lowering specificity. So we should write .tab__link only (without .tabs). Unfortunately, if we do that it will be overridden by higher specificity styling coming from Classy.
Also, it should be .tabs__link instead of .tab__link (see above).

Similar things about footer promo block. E.g. this does not follow our CSS coding standards about BEM:
.block-type-footer-promo-block .footer-promo-content .link
Ideally we would have something like .footer-promo__link only.

1. +++ b/core/profiles/demo_umami/themes/umami/css/components/navigation/tabs/tabs.css
   @@ -19,17 +19,17 @@
   +.tabs .tab__link {
   ...
   +.tabs .tab__link.is-active {
   ...
   +.tabs .tab__link:focus,
   +.tabs .tab__link:hover {
   

   Could we open a follow-up issue for the BEM adjustments as per #6?

2. +++ b/core/profiles/demo_umami/themes/umami/templates/field/field--link.html.twig
   @@ -0,0 +1,86 @@
   +          <a href="{{item.content['#url']}}" class="link">{{ item.content['#title'] }}</a>
   ...
   +        <a href="{{item.content['#url']}}" class="link">{{ item.content['#title'] }}</a>
   ...
   +        <a href="{{item.content['#url']}}" class="link">{{ item.content['#title'] }}</a>
   

   We should not read render array keys directly because it might cause problems with cacheability. See #2660002: Allow explicit bubbling of cacheability metadata inside Twig template (when accessing data from instead of rendering render arrays) for more background information.

@lauriii In the second part of your comment #7 you told:

We should not read render array keys directly because it might cause problems with cacheability. See #2660002: Allow explicit bubbling of cacheability metadata inside Twig template (when accessing data from instead of rendering render arrays) for more background information.

Is it true for a field templates? Because I was debugging cache tags and they where the same when I was using item.content or item.content['#url'] in the link field template.

If we should anyway do that for field templates, then regarding to the linked thread I can assume that I should use something like this in the link field template:

…
{% for item in items %}
      <div{{ attributes.addClass(classes, 'field__item') }}>
        {% set cache_cath = item.content|render %}
        <a href="{{item.content['#url']}}" class="link">{{ item.content['#title'] }}</a>
      </div>
{% endfor %}
…

Am I right? Thank you.

@hiway did you set any additional cacheability metadata in item.content? Core might not set any additional cacheability metadata there, but we cannot guarantee there isn't any custom code or contrib modules doing that.

Is there any way we could just render the full render array? If we could do that, we wouldn't even have to think about workarounds.

@lauriii Yes, you are right, we can try attach need classes in the field preprocess. I will create a new path soon. Thank you.

@lauriii I got a new patch which includes all mentions from previous comments from @thamas - #6 and from you #7. Please review these solution.

I changed this to "Needs Review" regarding to the changes in this comment #12

I've tested this one and it works. The a element selectors are nice replaced with bem-styled classes.

Overall this looks nice cleanup. It also seems like this already addresses #6 so we don't need to open a follow-up 👌

+++ b/core/profiles/demo_umami/themes/umami/umami.theme
@@ -38,6 +38,20 @@ function umami_preprocess_field(&$variables, $hook) {
+      $bem_block_class= 'link';

This is not used anywhere.

@auriii Thanks for that notice. I leaved variable and just used it in the classes definition loop, where the string was before:

// Iterate through the items if field is multiple,
// and attach classes.
foreach ($variables['items'] as $key => $item) {
  $variables['items'][$key]['content']['#options']['attributes']['class'][] = $bem_block_class;
  $variables['items'][$key]['content']['#options']['attributes']['class'][] = $bem_element_class;
}

Think that's the good way.

Changing it to to the "Needs Review" after last patch

I tested this in all D8 compatible browsers and the tabs and block links have been replaced by BEM classes and styles.

1. +++ b/core/profiles/demo_umami/themes/umami/umami.theme
   @@ -38,6 +38,20 @@ function umami_preprocess_field(&$variables, $hook) {
   +      // Prepare var to keep parent block name.
   ...
   +      // Define BEM block class name.
   ...
   +      // Define BEM element class name.
   ...
   +      // Iterate through the items if field is multiple,
   +      // and attach classes.
   

   We can remove these lines of inline documentation since they don't bring any new information on top of what can be seen quite easily from the code.

2. +++ b/core/profiles/demo_umami/themes/umami/umami.theme
   @@ -38,6 +38,20 @@ function umami_preprocess_field(&$variables, $hook) {
   +      $bem_block_class= 'link';
   ...
   +        $variables['items'][$key]['content']['#options']['attributes']['class'][] = $bem_block_class;
   

   Why are we adding this class? It is not being used anywhere.

@lauriii Regarding to your question, about why are we using this class:
$variables['items'][$key]['content']['#options']['attributes']['class'][] = $bem_block_class;
In current situation yes, this class is not used. But regarding to the BEM methodology principles the link itself can be a block and that class is the actual block name. In the real BEM projects any link is a block and usually the block name is "link", that's why I decided to keep that class there. But we can remove it if you really insist, so far we are using only naming conventions here.

@lauriii I removed not need comments and kink block class name.

@hiway

Please re-rolled the patch again so we can verify the same in 8.8.x version.

@pawandubey Here is the new patch.

Patch 27 works for me

All looks good so setting back to RTBC! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

RTBC+1 as the re-test also passed

There are still multiple instances where elements are used in selectors. The most concerning instance we should at least include in the scope is that we attach styles directly to button element in the base.css.

@lauriii The scope of this particular ticket was described in the description, and there's nothing about buttons and so on. So, may be better to create a big followup ticket which will cover all cases where we are using selectors in CSS rules? And as I understand, base.css file describes styles on the top level, saying, for global scope, so it may have selectors in rules to create default styles, may be not, but as far as this file is out of components, I think it's allowed to have selector rules there.

tagging for DrupalNorth 2019

@hiway the original comment in the parent issue that requested to go through all selectors that are not using classes. I'm not too keen to a certain method of getting that done - if there's a significant amount of instances that need changes, it might be better to collaborate in multiple issues. However, often times working on multiple issues creates unnecessary overhead. What we are trying to achieve is to have as few selectors as possible using element type for applying rules. This is required to make us compatible with BEM and to make maintenance easier.

And as I understand, base.css file describes styles on the top level, saying, for global scope, so it may have selectors in rules to create default styles, may be not, but as far as this file is out of components, I think it's allowed to have selector rules there.

That is correct, but we want that file to be as small as possible. According to BEM, we are actually not supposed to have this kind of file at all because it makes maintenance harder. For example, button is a commonly used element that shouldn't have styles attached based on the element type.

@lauriii, thanks for your answer. I see what you say, will try to manage our goals when will have a time to work on this issue.

Tagging for DrupalCamp Colorado 2019 (Sunday August 4)

@lauriii, I have a chance to revise the base.css for a theme to clean up general selector rules. Instead of the form elements we also have there styles for typography: h1-h6, ol-ul, blockquote, img. And also there are styles for html, body and a selectors. What should we do with these styles? Leave it there?
Thank you

@lauriii Regarding to your request about base.css, I refactored that file and deleted all form selectors from there and moved them to the appropriate form components CSS files, all element selectors there changed to class names.
So, guys, please take a look to the updated path.
Thank you.

Tagging for badcamp2019, thanks! (October 2-5)

Patch or MR doesn't apply anymore
The last patch or MR doesn't apply to the target branch, please reroll the code so that it can be reviewed by the automated testbot.

Moving to NW for the issue summary update requested in #37. If you could include what elements you are replacing with classes, would be very nice if it could use the default issue template.

Testing the actual code on umami install on D10. I checked the search bar for button class and that css is being used.
Could not find a textarea.

+1 for RTBC after the issue summary update.

Maybe we get this one in and then open a follow up to re-evaluate if additional changes are needed?

Agreed that it would be nice to land this BEM clean-up! Thank you for working on this @markconroy and @smustgrave! 🙏

1. +++ b/core/profiles/demo_umami/themes/umami/css/components/blocks/footer-promo/footer-promo.css
   @@ -16,6 +16,27 @@
   +.footer-promo-block__link {
   +  color: #fff;
   +  background-color: inherit;
   +  font-weight: 700;
   +}
   

   I'm not sure this is doing anything because it looks like there's another rule with selector .footer a that overrides this 🤔

2. +++ b/core/profiles/demo_umami/themes/umami/umami.libraries.yml
   @@ -25,6 +25,10 @@ global:
   +      css/components/forms/inputs.css: {}
   +      css/components/forms/select.css: {}
   ...
   +      css/components/forms/textarea.css: {}
   

   It looks like this is adding CSS files that do not exist 🤔

Addressed #63, attached interdiff and patch for same. Please review

Points have been addressed thanks!

This needs some more manual testing. There's at least one regression with the search input.

Before:

After:

I am working on this issue , will provide an patch soon. Thanks

Removing credit from #68 has nothing was added.

Reviewing the patch #72 on an Umami install and I'm getting the same screenshots as @markconroy so don't think anything broke.

Nice to see that this is shaping up! Few small comments that occurred when looking at the patch:

1. +++ b/core/profiles/demo_umami/themes/umami/css/components/blocks/footer-promo/footer-promo.css
   @@ -16,6 +16,27 @@
   +.footer a.footer-promo-block__link {
   ...
   +.footer a.footer-promo-block__link:focus,
   +footer a.footer-promo-block__link:hover {
   ...
   +.footer a.footer-promo-block__link:after {
   

   Couldn't these selectors be simplified just to .footer-promo-block__link?

2. +++ b/core/profiles/demo_umami/themes/umami/css/components/forms/form-items.css
   @@ -0,0 +1,48 @@
   +.form-label {
   

   We should add @file documentation here 😇

3. +++ b/core/profiles/demo_umami/themes/umami/umami.theme
   @@ -39,6 +39,13 @@ function umami_preprocess_field(&$variables, $hook) {
   +        $variables['items'][$key]['content']['#options']['attributes']['class'][] = $bem_element_class;
   

   The block class is added in core/profiles/demo_umami/themes/umami/templates/components/footer-promo-block/block--bundle--footer-promo-block.html.twig which means that for other bundles it's not guaranteed that the block class exists. Maybe we can just hard code this for the footer-promo-block use case?

I think after we have made these changes, this is ready to be committed 😇

Think this is good to be looked at again.

#76: If we'd want to get rid of those in the selector, we could duplicate the class selector. I.e. .footer-promo-block__link.footer-promo-block__link. We've used this in Claro because it allows increasing the selector specificity without adding additional dependencies.

Failure in #79 seems unrelated

Installed a fresh Umami install with patch #79 and the footer seemed to still function

"Improved" how the class gets added to the link. I don't know if this is too much of an improvement but at least it's more consistent with other places where we do similar things, and it seems less prone to issues when the field config gets modified.

Committed 161f201 and pushed to 10.1.x. Thanks!

Final manual testing was done in #81, removing tag.