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

Re-rolled to work against 10.1.x

Better interdiff

Coding standards fixes.

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.

Thanks @smustgrave.

I don't see any request in #37 to update the IS.

Given that this issue is more than 5 years old, I can't give much more info than what is currently in the IS - basically, "let's try to theme Umami by BEM CSS classes and not theme it by targeting html elements themselves".

I'd really like to get this merged before we end up with the latest patch being way out of sync with the theme and taking a couple more hours to re-roll. If there's any follow-ups, we can look after them in a follow-up issue, but in the meantime there's been great work done in this issue by @hiway and others.

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:

It looks like the patch is removing all the styling for input but not replacing it with other CSS (for .form-text, .form-email, etc). The specific class that we need to fix that regression is .form-search.

-input {
-  min-width: 100%;
-  max-width: 100%;
-  margin: 0.25rem 0;
-  padding: 0.8rem 0.4rem;
-  color: #000;
-  border: 2px solid #767775;
-  font-size: 1rem;
-}
-input:focus {
-  border: 2px solid #008068;
-}
-input[type="checkbox"] {
-  min-width: inherit;
-  max-width: none;
-}
-
-textarea {
-  padding: 0.8rem 0.4rem;
-  color: #000;
-  border: 2px solid #767775;
-}
-textarea:focus {
-  border: 2px solid #008068;
-}

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

Unassigning.

A further update to this patch:

1. Moves the form-element-label.html.twig file from templates > classy to templates > form.
2. Adds a class of .form-label to all labels and uses this class to theme labels
3. Adds CSS for textarea via the .form-textarea class
4. Adds CSS for inputs via the .form-text and .form-email classes
5. Adds CSS for search results form input via .search-form .form-search class

Side-by-side screenshots of the contact and search pages (the only pages regressed by the previous patch) attached.

Sorry, I forgot to upload the patch and interdiff ;-)

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.

@lauriii any chance we could get this committed? I want to create a follow-up issue to convert the theme to using css variables, but don't want to start any work on that until we have this in core.

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 😇

Thanks @lauriii

We need to leave the .footer a part of the CSS in the footer promo block, or else our css gets overridden by .footer a that is in the footer CSS file.

I've the other items fixed now.

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.

Good tip @lauriii. Thanks. Attached.

The last submitted patch, 79: 2936875-79.patch, failed testing. View results →

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.

Looks good to me, thanks @lauriii

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

Woo hoo! Thanks.

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

Automatically closed - issue fixed for 2 weeks with no activity.