Problem/Motivation

Remove non essential classes from core - the classes are in classy, cores templates should be as clean as possible.
js required classes should be prefixed to js- according to code standards
simpletest should be done with classy theme, not stark

See parent issue(s) and related issue(s) for details,

Proposed resolution

Twig Templates to modify

  • core/modules/system/templates/radios.html.twig
  • core/modules/system/templates/region.html.twig

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sivaji_Ganesh_Jojodae’s picture

Status: Active » Needs review
FileSize
1.38 KB
davidhernandez’s picture

Status: Needs review » Postponed

Postponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme for updates. Thanks.

davidhernandez’s picture

Title: Copy system templates r*.html.twig to Classy » Remove classes from system templates r*.html.twig
Issue summary: View changes
Status: Postponed » Needs work

Un-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.

mortendk’s picture

Status: Needs work » Needs review
FileSize
914 bytes

Status: Needs review » Needs work

The last submitted patch, 4: template-cleanup-r.diff, failed testing.

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Status: Needs work » Needs review
Issue tags: -cssbanana
FileSize
913 bytes

lets try this again

davidhernandez’s picture

The only CSS I see attached to form-radios is margin. I don't think I see anything using 'region' directly but I do see some JS using region-*, do we need to change those? For example, quickedit looks for region-content.

mortendk’s picture

@david sounds like we should do a cleanup on quickedit as a seperate issue, or eventually scopecreep this ?

davidhernandez’s picture

Well, I don't think we can commit anything here without accounting for it, otherwise we introduce a regression.

mortendk’s picture

Status: Needs review » Needs work
mortendk’s picture

Issue tags: +jsbanana
mortendk’s picture

I would suggest that we then do a css standards on quicklinks cleanup & then at the same time there fixes any calls to region-content.
- yes more followups i know, but :/

mortendk’s picture

Diggin into this:

    // In the case of a full entity view page, the entity title is rendered
    // outside of "the entity DOM node": it's rendered as the page title. So in
    // this case, we must find the entity in the mandatory "content" region.
    if (entityElement.length === 0) {
      entityElement = $('.region-content').find(entityElementSelector);
    }

Could somebody point to the place that says that drupal have to have a "content" region - which is ok to assume.
But if that region is there then we assume that offcourse we have a wrapper around it thats called .region-contentt around the region
It seems like were requiring themes to have this, if thats the case - Then it should be documented somewhere & the classnames must indicate this as well in this case with a js- prefix - If this is not the case we should clean this up.

davidhernandez’s picture

Could somebody point to the place that says that drupal have to have a "content" region - which is ok to assume.

https://www.drupal.org/node/171224

In Drupal 7, $content became a full region and is now mandatory in all themes. This new requirement was set up so that when enabling new themes, Drupal knows where to put the main page content by default.

I knew this was true for 7, but have never checked to see if it was a hard requirement for 8.

mortendk’s picture

Status: Needs work » Needs review
Issue tags: -jsbanana
FileSize
1.51 KB
534 bytes

ok cleaned it up a bit and removed the .region-content selector, as we cant rely on that whatsoever to be a part of a theme (usually one of the first places anybody goes to cleanup - when the div soup is to thick) I changed the selected from region-content to be main instead.

We should use <main> instead of .region-content - Based on the following reasons:
* using css for this is terrible - ill keep the rant down! but .... *grrr*
* Main should always be present on a page, its like having a head / footer so it makes perfect sense.
* Main is an unique tag that there should be only once on a page (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main) - hurray without using an ID

This should be documented somewhere in the big book of "what goes on in drupals frontend", but maybe in the page.html.twig as well ?

davidhernandez’s picture

+++ b/core/modules/quickedit/js/quickedit.js
@@ -272,7 +272,7 @@
     // outside of "the entity DOM node": it's rendered as the page title. So in
     // this case, we must find the entity in the mandatory "content" region.
     if (entityElement.length === 0) {
-      entityElement = $('.region-content').find(entityElementSelector);
+      entityElement = $('main').find(entityElementSelector);

Should change that comment about using "content" region, since it is being changed to <main>.

Also, this needs some testing to make sure it is still working right, whatever it does. This entityElement, it is being moved from the region-content div to the main tag which is two levels up in the markup.

mortendk’s picture

FileSize
1.82 KB

Selector changed to be main[role="main"] which is the default wrapper for "content" and the full view of a node
In that way we dont "sneak in" a dependency on .region-content that nobody have an idea about and <main role="main"> is always present.

mortendk’s picture

Issue tags: +dclondon
rteijeiro’s picture

Fixed a typo in a comment.

rteijeiro’s picture

Issue summary: View changes
FileSize
187.1 KB
186.93 KB
70.6 KB
70.79 KB

Attached a few screenshots:

Radios BEFORE

Radios AFTER

QuickEdit BEFORE

QuickEdit AFTER

mortendk’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -dclondon

CR updated as well

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/quickedit/js/quickedit.js
@@ -270,9 +270,11 @@
+      // entityElement = $('main').find(entityElementSelector);

Looks like this can be removed.

lduerig’s picture

new patch

lduerig’s picture

Status: Needs work » Needs review
lduerig’s picture

FileSize
531 bytes
rteijeiro’s picture

Status: Needs review » Needs work
+++ b/core/modules/quickedit/js/quickedit.js
@@ -270,9 +270,10 @@
+       entityElement = $('main[role="main"]').find(entityElementSelector);

Wrong indentation.

The last submitted patch, 24: template-cleanup-r-2407739-24.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
1.77 KB

Just fixing that indent.

For this change, I'd be real careful about checking all the region stuff. That's what worries me in possibly introducing a regression. Make sure to test with Stark.

mortendk’s picture

Assigned: Unassigned » mortendk
Issue tags: +Needs screenshots

@david

+++ b/core/modules/quickedit/js/quickedit.js
@@ -270,9 +270,10 @@
-      entityElement = $('.region-content').find(entityElementSelector);
+      entityElement = $('main[role="main"]').find(entityElementSelector);

We change the selector from using the class .region-content thats only in the region thats named "content" & move that up to <main role=" main"> thats unique.

im kinda thinking that we maybe should add a data-attibute, or adding in a class lik "js-quickedit-selector-foo" to make it more robust + needs comments in the source so its clear that we use this as a selector for quickedit

But yes needs test anyways

davidhernandez’s picture

Assigned: mortendk » Unassigned
FileSize
1.08 KB
2.28 KB

I'm no JS wiz, but this seems to maintain the quickedit functionality.

mortendk’s picture

we should really use a js-quickedit class here else nobody have an idea of whats going on

mortendk’s picture

FileSize
347.47 KB
338.59 KB
401.91 KB
321.24 KB
4.03 KB

@david think one of the win we could get with removing the quickedit dependency from the region template, we could make it more obvious where it comes from and whats happening in the code. In that light i would suggest that we put it out in the main region and used .js-quickselect to make it clear for the themer whats going on + we dont fill all the region divs with data-- that will probably scare a lot of people.

a reroll + manual testing of all existing themes.

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: -Needs screenshots
+++ b/core/modules/quickedit/js/quickedit.js
@@ -369,9 +369,11 @@
+      entityElement = $('.js-quickedit').find(entityElementSelector);      ¶

I think this is a good idea. Can we change this class to something more meaningful? This isn't a trigger for all of quickedit, this is a specific selector for the main content container, how about .js-quickedit-main-content? Also there is whitespace at the end of this line.

mortendk’s picture

Status: Needs work » Needs review
FileSize
4 KB

@lewis good point ive updated the patch

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I manually tested this and I'm happy with the changes here. I wave the RTBC wand.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Markup is not frozen yet. Committed 22cd7b8 and pushed to 8.0.x. Thanks!

  • alexpott committed 22cd7b8 on 8.0.x
    Issue #2407739 by mortendk, davidhernandez, rteijeiro, lduerig, sivaji@...
tstoeckler’s picture

Wow, it's very strange to have a reference to quickedit in System module's page template.

I can see 2 ways to fix this (IMO) "properly".

1. Decide that finding the main-content is useful for all kinds of JS and call this "js-main-content"
2. Have Quickedit module add that in preprocess (this would require the addition of a variable for that I guess)

LewisNyman’s picture

2. Have Quickedit module add that in preprocess (this would require the addition of a variable for that I guess)

I guess this makes the most sense. This is why we allow modules to inject classes.

davidhernandez’s picture

I think we'd need an attribute object for that. 'page' doesn't currently have one for any of the other elements. I wonder if that is just a coincidence or there is a reason.

mortendk’s picture

its even stranger to have the selector hidden inside a regions classes with no references to it ;)

Seriously lets not have preprocess add in all kinds of stuff, just so we can hide it, its been an epic pita for years, that so much have been hidden.
yes we allow module to add in classes, but really they should not do that, its a fallback for when its not possible otherwise, its keeping us in the "preprocess for everything" thinking of drupal7 - lets get it out in the open so we can see & look at it, yes this is TX to the max.

Adding in {{ attributes }} ... hmmm yes but we have em all over and nobody knows what they are used for, they are just there, so unless theres a really good reason for that, lets not confuse ourself with this just to create anther box where we maybe can dump stuff into.

Wim Leers’s picture

Status: Fixed » Active
+++ b/core/modules/system/templates/page.html.twig
@@ -96,7 +96,7 @@
-  <main role="main">
+  <main role="main" class="js-quickedit-main-content">

I just discovered this by accident, I was also very surprised by this.

+++ b/core/modules/quickedit/js/quickedit.js
@@ -369,9 +369,10 @@
+    // this case, we find the main element of the page and use it as a full
+    // entity ancestor.

What does "full entity ancestor" even mean? I wish you'd have pinged a Quick Edit maintainer for review.

yes we allow module to add in classes, but really they should not do that

Huh? By that reasoning, certain modules would never be able to function. They need to inject classes/markup in order to provide certain functionality.

The problem is that we can rely neither on:

  • a selector for the "main content" region (that dependency was removed in this patch)
  • <main> being present (not every theme may have this)
  • [role=main] being present (not every theme may have this)
  • .page-title being present (some themes may just use <h1>)

Quick Edit just needs to know the DOM node in which the other fields of the entity besides the title live. Perhaps there's another way possible now, compared to 3 years ago?

mortendk’s picture

Huh? By that reasoning, certain modules would never be able to function. They need to inject classes/markup in order to provide certain functionality.

Yes certain modules may need to do this, but we should work hard to have all classes inside of the theme instead of hidden a few away in a module.
It makes it hard to find for a themer & makes it a pita to track where stuff comes from.
It should always be the exception to add a class from a module.

main ftw
actually we can rely on themes having the main tag, the same way as we can rely on them having a body or footer.
<main> should always be present in a page - not having a role="main" would also go against the accesability
https://developer.mozilla.org/en/docs/Web/HTML/Element/main

.page-title yes please let us remove that, its one of these.

Wim Leers’s picture

actually we can rely on themes having the main tag, the same way as we can rely on them having a body or footer.

Perfect! In that case, Quick Edit can just rely on that! Much simpler. None of the ugliness introduced here is necessary anymore then.

Patch at: #2568099: Follow-up for #2407739: Remove the js-quickedit-main-content class that was added in favor of relying on <main>. Let's clean this up :)

  • webchick committed e36c84d on 8.0.x
    Issue #2568099 by Wim Leers, mdrummond, mortendk: Follow-up for #2407739...
Wim Leers’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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