Follow up from #1982256: Clean up html.html.twig markup

Meta issue: #1980004: [meta] Creating Dream Markup

Problem/Motivation

The body's classes do not follow Drupal 8's CSS coding standards, we have classes that have never been used: .html, strange empty classes (page-node-), etc.

html 
front / not-front 
not-logged-in / logged-in
in-maintenance
page-node 
page-node- 
page-node-$nid
...

Proposed resolution

Drupal will provide the following classes as default in <body>:

.path-frontpage (if on the frontpage)
.path-$path (first part of the path - .path-admin, .path-node)
.user-logged-in
.page-maintenance (existing class to use instead of in-maintenance)

Remaining tasks

None

User interface changes

n/a

API changes

Markup/CSS class changes only.

Files: 
CommentFileSizeAuthor
#134 change_the_body_class-2234331-134.patch8.67 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,406 pass(es). View
#134 interdiff-2234331-129-134.txt2.12 KBlauriii

Comments

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Title: clean up classes in the body element » body classes
FileSize
5.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,522 pass(es), 2 fail(s), and 0 exception(s). View
mortendk’s picture

Status: Active » Needs review
mortendk’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: bodyclass-2234331-1.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,469 pass(es). View
620 bytes
paolomainardi’s picture

I guess we should handle also taxonomy and user pages as well, what do you think ?

nicholasruunu’s picture

The sidebar classes can be quite handy to transform the layout based on using/not using the sidebar.
Something simpler than what we had could probably be used.

Also now we're losing the admin class which could also be handy while styling inline admin stuff.

Was there any other functionality with the class suggestions we're missing now?

mortendk’s picture

@paolomainardi
i dont think that drupal out of core should handle all situations it probablly could end up with, themes have a preprocess function for a reason.
Lets follow the 90% idea, at not end up in endless "what-if"' ask your self: Do I always have a need for a class to identify a user page or the taxonomy page ? - or is that done in another place on the page.

a usecase could be to have a .page--$first-path-of-the-url that would give us out the box a path to different sections

.page--admin
.page--$section
.page--user

@nicholasruunu
Yup the sidebar is handy - That is if your theme is build like a 3 column site & the sidebars are called sidebar_first sidebar_last, if you dont then its just cruft. Bartik & Seven both have these build in, and they are defined in the .theme preprocess

I think is documentation issue on how to make your theme understand what regions are visibile, in that way Drupal dosnt take any desissions about your design and we dont lock anybody into the 3 columsn is what you have to work with. By keeping these inside of core there will be confusion on wtf is going on.

The .admin class sounds like a 90% issue :)

paolomainardi’s picture

@mortendk

Yes, i agree with you, i think that a class like ".page--$first-path-of-the-url" would cover enough.

wiifm’s picture

Status: Needs review » Needs work

Heya @mortendk!

Some thoughts on your patch:

  1. +++ b/core/includes/theme.inc
    @@ -1947,25 +1947,17 @@ function template_preprocess_html(&$variables) {
    -  $body_classes[] = 'html';
    

    great change, why was that even there in the first place.

  2. +++ b/core/includes/theme.inc
    @@ -1947,25 +1947,17 @@ function template_preprocess_html(&$variables) {
    +  $body_classes[] = 'path--' . implode("-",arg() );
    

    watch the 80 char wrapping around here, also there is weird spacing issues around arg()

  3. +++ b/core/includes/theme.inc
    @@ -2296,21 +2271,11 @@ function template_preprocess_maintenance_page(&$variables) {
    +  $variables['attributes']['class'][] = 'page--maintenance-in-maintenance';
    

    Is page--maintenance enough, why do we need page--maintenance-in-maintenance as well?

Happy class killing ;)

dead_arm’s picture

I've been outspoken about removing body classes, and I'm happy as long as the ones proposed make it in.

mortendk’s picture

@dead_arm exactly we need to fine good sensible compromises :)

LewisNyman’s picture

I think we could get rid of .user-logged-in--not because we could just use the :not() CSS selector here?

If we do use it, it should more likely be: .user-not-logged-in because you wouldn't expect to inherit styles from .user-logged-in. Actualy the two can't exist together!

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Status: Needs work » Needs review
FileSize
3.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,121 pass(es). View

ok heres an update on the body classes:

* Removed the .front / .not-front classes as we have that with .path--frontpage
* Removed user-not-logged-in as lewis said these can be target with :not

body.path-frontpage{ background:red;}
body:not(.path-frontpage){background:yellow;}
body.user-logged-in{ background: pink;}
body:not(.user-logged-in){background:yellow;}

Instead of the page-$something, renamed it to .path instead (cause it make sense), path-frontpage is added for the frontpage (and .front is removed) for consistency.
.node-type is renamed to .content-type--[foo] cause its a content type (let the bikeshed begin)

.path-frontpage 
.path-[first-of-the-path]
.user-logged-in
.content-type--[article]
.page--maintenance + .page--maintenance--active

so to add a pink background for admin would be .path-admin{background:pink}

GemVinny’s picture

Yeah I agree with Lewis about using :not.

Does there need to be both .page--maintenance /.page--maintenance-in-maintenance ?

Also mortendk you didn't change node-type to content-type. Want me to do it?

GemVinny’s picture

One more thing... it wasn't .page-maintenance--active that you added, it was .page--maintenance-in-maintenance ;-)

mortendk’s picture

FileSize
3.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,146 pass(es). View
999 bytes

@lilgemvinny + @wiifm
spot on no reason for 2 diffeent classes for the maintanence :)

mortendk’s picture

Issue summary: View changes
LewisNyman’s picture

.node-type is renamed to .content-type--[foo] cause its a content type

I support this proposal.

+++ b/core/includes/theme.inc
@@ -2242,8 +2242,7 @@
+  $classes[] = 'page--maintenance';

It feels like we're using variants a bit inconsistently right now. We should only be using them if there is an original CSS component that is modified, not just for any adjective that describes something. So if you have '.page--maintenance' you would expect to find '.page' which would have some styling applied to it. But there is not :-)

mortendk’s picture

@lewis
yup somehow that sneaked it self in - gonna run another clean on it later

mortendk’s picture

FileSize
4.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,301 pass(es). View

removed the wrongly used modifier -- fron the classnames

mortendk’s picture

Issue summary: View changes
dead_arm’s picture

Tested patch in #23, and getting classes as desired, except page-maintenance when in maintenance mode. Attached screenshot. Not sure if this is because of a core bug.

Also need to clean up patch to follow core code standards for php before committing.

tim.plunkett’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1971,25 +1971,22 @@ function template_preprocess_html(&$variables) {
    +  $body_classes[] = $variables['logged_in'] ? 'user-logged-in' : '';
    

    I think this '' is causing an extra space in the classes array.

  2. +++ b/core/includes/theme.inc
    @@ -1971,25 +1971,22 @@ function template_preprocess_html(&$variables) {
    +  // Add a class that tells us where in the path we are
    +  $path = arg();
    +  if( empty($path['0']) ){
    +    $body_classes[] = 'path-frontpage' ;
    +  }else{
    +    $body_classes[] = 'path-' . $path['0'] ;
    +  }
    

    We're working to remove arg, so I'm not sure we want to do this. Also the PHP standards are off here, need a space after if, no space in the (), and the else should be on its own line

  3. +++ b/core/modules/node/node.module
    @@ -583,9 +583,9 @@ function node_is_page(NodeInterface $node) {
    -  // If on an individual node page, add the node type to body classes.
    +  // If on an individual node page, add the node type to body classes
    

    No change needed here.

  4. +++ b/core/modules/node/node.module
    @@ -583,9 +583,9 @@ function node_is_page(NodeInterface $node) {
    -    $variables['attributes']['class'][] = drupal_html_class('node-type-' . $node->getType());
    +    $variables['attributes']['class'][] = drupal_html_class('content-type-' . $node->getType());
    

    This rename isn't okay. Even though we refer to Content Type in the UI, everywhere in code it is a node type.

mortendk’s picture

FileSize
3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,650 pass(es). View
4.61 KB

@deadarm the maintainance-page is only set if you dont have acces to the pages. guess thats by design ? Dont know if it make sense to add it in, as the page for people that dont have acces to the site, have a page thats completely stripped from content (open up a chrome browser in anonymous mode & you get the empty page)

@timp well if we dont know whats gonna happen with arg(0) then let us fix that when / if its gonna happen - what were doing in this patch is cleaning up the stuff that we have allready & is creating unusable classes like .page-foo- .

alright rerolled it and removed the dumb stuff ;)

LewisNyman’s picture

Status: Needs review » Needs work

Guys! I think I'm reviewing PHP! What's going on?

  1. +++ b/core/includes/theme.inc
    @@ -1995,27 +1995,27 @@ function template_preprocess_html(&$variables) {
    +  //  type, etc.).
    

    One too many spaces here

  2. +++ b/core/includes/theme.inc
    @@ -1995,27 +1995,27 @@ function template_preprocess_html(&$variables) {
    +    $body_classes[] = 'user-logged-in' ;
    ...
    +    $body_classes[] = 'path-frontpage' ;
    

    I've never seen spaces before semicolons anywhere else in core?

  3. +++ b/core/includes/theme.inc
    @@ -1995,27 +1995,27 @@ function template_preprocess_html(&$variables) {
    +  }else{
    +    $body_classes[] = 'path-' . $path['0'] ;
    +  }
    

    We need a space between } and else.

mortendk’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,612 pass(es). View
864 bytes

dear @LewisNyman yes you can totally do php - you are allowed to bye alchohol - then your grown up enough to do php ;)
anyways i cleaned up my codemess - so here ya gogo (wake me up)

LewisNyman’s picture

Status: Needs review » Needs work
+++ b/core/includes/theme.inc
@@ -1995,27 +1995,27 @@ function template_preprocess_html(&$variables) {
+  // This allows advanced theming based on context (home page, node of certain
+  //  type, etc.).

Sorry dude but you missed this space here

mortendk’s picture

Status: Needs work » Needs review
FileSize
490 bytes
3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,621 pass(es). View

1 less spase

idflood’s picture

Sorry for yet another whitespace nitpick. According to https://drupal.org/coding-standards#controlstruct there is still some little coding standard issue (unless this changed for d8):

if( $variables['logged_in'] ) {
// should be
if ($variables['logged_in']) {
if( empty($path['0']) ) {
// should be
if (empty($path['0'])) {
} else {
// should be
}
else {
dawehner’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1995,27 +1995,27 @@ function template_preprocess_html(&$variables) {
    +  $path = arg();
    ...
    +  } else {
    +    $body_classes[] = 'path-' . $path['0'];
    +  }
    

    Can't explain how embarassed I am about that ... do we want to actually use the internal path or the path alias as well?

  2. +++ b/core/includes/theme.inc
    @@ -1995,27 +1995,27 @@ function template_preprocess_html(&$variables) {
    +  if( empty($path['0']) ) {
    +    $body_classes[] = 'path-frontpage';
    

    We do have drupal_is_front_page for that

mortendk’s picture

FileSize
3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,626 pass(es). View
786 bytes

@dawehner : I think we wanna use the path alias, the internal dosnt matter - its what the themer can see in the adress bar , cause that make sense.
so your saying to not use arg(0) - then what should we use instead (me shwoing no knowledge of d8:)

mortendk’s picture

FileSize
3.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,624 pass(es). View
661 bytes

this patch
removed the call on arg(0) concerning tims commens using request_path() instead
Testing on drupal_is_front_page() for frontpage class as well

dawehner’s picture

Nope, $request->getPathInfo() is certainly more the thing you wanna use.

mortendk’s picture

FileSize
3.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,341 pass(es), 20 fail(s), and 0 exception(s). View
804 bytes

using $path = \Drupal::request()->getPathInfo(); now for the gods of oop

opening up a bikeshed:
Wondering if .path-foo is the right classname?
it make sense in the case of its the path but in a normal speaking term would it make more sense to have use "page" or "section" instead ?
I dont think that Core needs to think all these options in its a thing for a basetheme.

so can i get a thumps up / down for using .path-$foo as the basic classname, or something else that would make sense.
hope to close the bikeshed soon ;)

joelpittet’s picture

IMO, thumbs down, don't include the path/url parts at all in the classes.

Maybe you can give some good examples of the big overarching benefits this gives us over just using the node-type?

Here's how I see this going from handy to OMG, why did we do this?

.path-admin // Fine
.path-user // ok
.path-news // kinda helpful
.path-contact // kinda helpful
.path-about-company // kinda helpful
.path-really-long-title-with-no-pathauto-prefix-section // Kinda unhelpful
.path-really%20-awkward-%21%40%23%24-encoded-alias // Yikes!

Also if you still think this is a good idea, make sure you don't get language prefixes /fr /ca /us etc resulting in

.page-us
.page-ca  
.page-fr

for all your paths. So needs tests for the above.

aspilicious’s picture

Yeah I'm not found of the url alias either. The internal ID ==> ok, the alias ==> no go.

Status: Needs review » Needs work

The last submitted patch, 37: bodyclass-2234331-37.patch, failed testing.

mortendk’s picture

Sorry gentlemen i do think your both wrong(tm) The reason is that we creating a system that besides of being as clean as we can make it, also want to have sensible defaults.

nid: Using $nid is not a reliable usecase - using node id's for anything is a disaster waiting to happen (and the reason we removed em from node.html.twig) and do not fall down to a even 15% usecase. actually its one of the reasons for this patch.

path classes
Using the first bit of the path is as joel actually shows in his example is a 90% usecase, and honestly just make sense Just because a path can not be usable sometimes (as the example of multilangual sites, where the site is not using subdomains) is not the same as its not usable to have the path, or "section" as its called in zen theme, or pathone-, .page-, etc as its called other places.

.path-admin // make sense
.path-user // make sense
.path-news // stll make sense
.path-contact // yes makesende
.path-about-company // yup more sense
.path-really-long-title-with-no-pathauto-prefix-section // we cant take care of all but still make sense
.path-really%20-awkward-%21%40%23%24-encoded-alias // and not even the dumb but make sense

Having a body class based on the name of the first path makes sense to 90% of the themers out there.

My bikeshed question was should it be called .path-$foo or .page-$foo not do we even want it there, cause we do as it plan just make sense & is not cruft.

aspilicious’s picture

The problem with aliases is the following, aliases can change, id's can't.
So if the client decides to rename the contact page and change it's url your page is potentially screwed.
By setting the default to alias you give themers "the impression" that they can safely use those classes.

I'm not going to fight this as I'm not going to be the guy theming the pages, but I'll be the guy that has the to explain to the client why the layout of the page gets screwed when they changed the title.

Anyway, have fun finishing this patch :)

joelpittet’s picture

I'm not for ID's either BTW they are kinda safe as they won't change often but they may change between dev,stage, production environments, so glad they are gone and don't give false hopes to themers. So there are other solutions for the uniqueness that can be applied as needed. Like page based classes, or panels, or context module classes, etc...

The path alias seems to be trying to solve the "site section" based styling. Though I'd rather leave that up to the themer/base_themer to come up with the pattern proposed and not include this in core. One less class pattern to worry about and leave it up to the creativity of the themer to deal with how these extra classes should be handled.

mortendk’s picture

@joelpittet
Yup exactly its the section issue + a bit of quick n easy solution that were trying to sold (well actually its more cleaning up in all the baseclasses and at the same time streamline the .page-, .page-$nid, .page-$section that im trying to solve. Simply cause its a very usable class to have, for all kinds of hackery.

Maybe its there more as a convinience aka "we always had it" argument.

Anyways the reason for this path is not to remove the .page class its to cleanup the bodyclasses.

mdrummond’s picture

If we're talking about whether it's better to have path-foo or page-foo as a class, I'd say path-foo is better. That makes it clearer what the class name is all about. I don't like page-foo, as there is a lot of content on Drupal that isn't a "page". The more we can get away from everything being a page the better, in my opinion.

One other note. Am I understanding this correctly that foo represents only the first part of the path before any additional slashes? If that's the case, Maybe path-root-foo would be clearer. Although if that is the case that does make me question the usefulness of this class, since it would only be useful for changing styles based on top-level sections.

All I know is that I vote path-foo over page-foo on this issue.

dawehner’s picture

+++ b/core/includes/theme.inc
@@ -1995,27 +1995,29 @@ function template_preprocess_html(&$variables) {
+  $path = \Drupal::request()->getPathInfo();
...
+    $body_classes[] = 'path-' . $segment[1];

We should use drupal_html_class here.

I guess we want to have at least some really basic test coverage.

LewisNyman’s picture

Issue tags: +frontend
LewisNyman’s picture

Issue tags: +focus

Let's figure this out in Austin

mortendk’s picture

Assigned: Unassigned » mortendk
Status: Needs work » Needs review
FileSize
3.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,881 pass(es). View

reroll & claimed

aburrows’s picture

Assigned: mortendk » aburrows

just reviewing now :)

aburrows’s picture

Issue summary: View changes

There was 1 class not defined correctly during testing: maintenance-page are we working with page-maintenance?

mortendk’s picture

we use the the word "maintenance-page" as it cause that what it is - its a maintenance page ;)
the whole in-maintenance etc is left overs from the good old days

aburrows’s picture

Status: Needs review » Reviewed & tested by the community

Awesome in which case this patch works as intended ;)

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.inc
@@ -1975,27 +1975,31 @@ function template_preprocess_html(&$variables) {
+  if($variables['logged_in']) {
...
+  if(drupal_is_front_page()){
...
+  }else{

This needs to follow coding standards. spaces around the if, and else on new lines.

Also, this issue needs a real title.

mortendk’s picture

Title: body classes » change the body classes to follow drupals css standards
mortendk’s picture

Issue tags: -frontend, -focus
FileSize
3.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch bodyclass-2234331-56.patch. Unable to apply patch. See the log in the details link for more information. View
mortendk’s picture

Status: Needs work » Needs review

testbot engage!

mortendk’s picture

Assigned: aburrows » mortendk

gonna hunt this one down

mortendk’s picture

Title: change the body classes to follow drupals css standards » make the body class follow drupals css standards & remove layout classes for stark
Issue tags: +Stark, +Bartik
mortendk’s picture

Title: make the body class follow drupals css standards & remove layout classes for stark » change the body class follow drupals css standards & remove layout classes for stark
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I had a look over this to check for regressions in Stark and Bartik and I couldn't find anything.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 56: bodyclass-2234331-56.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 56: bodyclass-2234331-56.patch, failed testing.

sqndr’s picture

Issue tags: +needs-reroll

The last patch does not apply any more, it needs a reroll.

sqndr’s picture

Status: Needs work » Needs review
FileSize
3.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,730 pass(es). View

Here's that reroll. Hopefully testbot will be happy now!

mortendk’s picture

testboot is happy! :)

Cottser’s picture

Issue tags: -needs-reroll
sqndr’s picture

FileSize
3.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch bodyclass-2234331-69.patch. Unable to apply patch. See the log in the details link for more information. View

Here's a new patch - it needed another reroll.

sqndr queued 69: bodyclass-2234331-69.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 69: bodyclass-2234331-69.patch, failed testing.

sqndr’s picture

Issue tags: +needs-reroll
superspring’s picture

Assigned: mortendk » Unassigned
Issue tags: -needs-reroll +Needs reroll

Freeing this up for someone else...

amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,425 pass(es). View

Reroll of #69.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I manually tested that all the classes should appear as described in the issue summary again. Looks good!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need a change record for this.

+++ b/core/themes/seven/css/maintenance-page.css
@@ -134,12 +134,12 @@
   .in-maintenance,
-  .install-page {
+  .maintenance-page {

Do we need the .in-maintenance here now? We might be able to drop

  $classes[] = 'in-maintenance';

from template_preprocess_maintenance_page too?

joelpittet’s picture

A few more little things to take care of in this patch.

  1. +++ b/core/includes/theme.inc
    @@ -2016,27 +2016,32 @@ function template_preprocess_html(&$variables) {
    +  // etc.) as well as more specific data like path-frontpage or
    +  // path-$contenttype.
    +  $path = \Drupal::request()->getPathInfo();
    

    This doesn't do this path-$contenttype. Should this doc line be removed?

  2. +++ b/core/includes/theme.inc
    @@ -2016,27 +2016,32 @@ function template_preprocess_html(&$variables) {
    +    $segment = explode("/", $path);
    

    This should be single quotes.

  3. +++ b/core/includes/theme.inc
    @@ -2016,27 +2016,32 @@ function template_preprocess_html(&$variables) {
       $path_args = explode('/', current_path());
    

    This variable is not being used and the line can be removed as well.

  4. +++ b/core/includes/theme.inc
    @@ -2289,7 +2285,6 @@ function template_preprocess_maintenance_page(&$variables) {
    -  $classes[] = 'maintenance-page';
    
    +++ b/core/themes/seven/css/maintenance-page.css
    @@ -15,7 +15,7 @@
    -.in-maintenance {
    +.maintenance-page {
    
    @@ -134,12 +134,12 @@
    -  .install-page {
    +  .maintenance-page {
    ...
    -  html, .install-page, .in-maintenance {
    +  html, .install-page, .maintenance-page {
    

    Seems .in-maintenance is being used instead of .maintenance-page {}

    Should this be replaced from bartik's:

    core/themes/bartik/css/maintenance-page.css
    

    or if that was meant to be like the issue summary and .maintenance-page is used, it should be replaced in seven's:

    core/themes/seven/css/maintenance-page.css
    core/themes/seven/css/install-page.css
    
lauriii’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
4.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,036 pass(es), 96 fail(s), and 47 exception(s). View

I fixed points pointed by @joelpittet in #77. I also changed .in-maintenance to .maintenance-page like the issue summary says.

joelpittet’s picture

Thanks for the fixes @lauriii! Everything looks perfect except one typo:

+++ b/core/includes/theme.inc
@@ -2055,7 +2052,7 @@ function template_preprocess_maintenance_page(&$variables) {
-  $classes[] = 'in-maintenance';
+  $classes = 'maintenance-page';

This still needs to be an array.

lauriii’s picture

FileSize
553 bytes
4.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,234 pass(es). View

Ups.. Changed it back to an array :)

The last submitted patch, 78: body_classes-2234331-78.patch, failed testing.

G-raph’s picture

Assigned: Unassigned » G-raph
Issue tags: +frontendunited

I'm going to review this.

rteijeiro’s picture

Issue tags: -frontendunited +FUDK

It seems official tag is FUDK (kinda weird, BTW)

G-raph’s picture

Assigned: G-raph » Unassigned
FileSize
67.16 KB
65.4 KB

I installed a minimal drupal8 and activated the Stark theme.
Initial bodyclasses: html, not-front, logged-in, page-user, page-user-, page-user-%id
Then I installed the patch from comment #80.
After patching the bodyclasses were: user-logged-in, path-user

Everything worked fine with no errors.
Screenshots (before and after) in attach.

G-raph’s picture

Status: Needs review » Needs work
diff --git a/core/themes/seven/css/maintenance-page.css b/core/themes/seven/css/maintenance-page.css
index 213c134..e6a554c 100644
@@ -133,13 +133,12 @@
   html {
     display: table;
   }
-  .in-maintenance,
-  .install-page {
+  .maintenance-page {
     display: table-cell;
     padding: 1em 0;
     vertical-align: middle;
   }

.install-page is out of scope from this issue, but the styling on this class is removed.

All the rest is OK by me.

mortendk’s picture

Issue tags: +needs screenshots

Just to be the devils advocat we need screenshots also from :
Seven
Bartik

mortendk’s picture

Just to be the devils advocat we need screenshots also from :
Seven
Bartik

G-raph’s picture

Status: Needs work » Needs review
FileSize
4.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,868 pass(es). View
373 bytes

I fixed my own remark from #85.

G-raph’s picture

I created screenshots (before and after) from Bartik and Seven themes, this was asked in #86. Everything is OK.

I do have 1 remark about the contenttype-bodyclass: in the summary there is a class defenition of ".content-type-$nodetype", but after testing the bodyclass is "node--type-$nodetype". (but I think this will be OK)

lauriii’s picture

Maintenance page seems to work also! We still need a change record.

GemVinny’s picture

Assigned: Unassigned » GemVinny

I'm going to add a change record.

jwilson3’s picture

Per #89.

GemVinny’s picture

Assigned: GemVinny » Unassigned
Issue tags: -Needs issue summary update

I have created a change record for this.

GemVinny’s picture

rteijeiro’s picture

Issue summary: View changes

Fixed issue summary.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record, -needs screenshots

Reviewed change record and seems good.

Also there are screenshots provided in #89 and #90.

Is it RTBC now?

sqndr’s picture

This looks good to me now as well.

Cottser’s picture

I updated the change record to more accurately reflect what's happening in the latest patch, thanks for getting it going @lilGemVinny!

+++ b/core/includes/theme.inc
@@ -1779,15 +1781,6 @@ function template_preprocess_page(&$variables) {
-  // Set up layout variable.
-  $variables['layout'] = 'none';
-  if (!empty($variables['page']['sidebar_first'])) {
-    $variables['layout'] = 'first';
-  }
-  if (!empty($variables['page']['sidebar_second'])) {
-    $variables['layout'] = ($variables['layout'] == 'first') ? 'both' : 'second';
-  }
-

This jumped out at me, but this variable appears to be completely unused and undocumented. Looks safe to remove but might be safer to do so in a separate issue.

Cottser’s picture

Title: change the body class follow drupals css standards & remove layout classes for stark » Change the body classes to follow Drupal 8 CSS standards

Also I don't think we're doing anything with the layout classes, they look to be already gone (or I'm missing something but the patch certainly doesn't seem to be doing anything with them). Removing that from the title and making it more title-y.

Cottser’s picture

Issue summary: View changes

Updating the issue summary.

mdrummond’s picture

The layout classes are the ones you referenced in #98 I believe.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
git ac https://www.drupal.org/files/issues/change_the_body_class-2234331-88.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4402  100  4402    0     0   3606      0  0:00:01  0:00:01 --:--:--  5989
error: core/themes/seven/css/install-page.css: does not exist in index
error: core/themes/seven/css/maintenance-page.css: does not exist in index
LewisNyman’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,186 pass(es). View
mdrummond’s picture

Only thing that changed in last patch is the file names. Patch still applies. Back to RTBC.

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

Actually back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  // Build a list of suggested theme hooks or body classes in order of
  // specificity. One suggestion is made for every element of the current path,
  // though numeric elements are not carried to subsequent suggestions. For
  // example, for $base='page', http://www.example.com/node/1/edit would result
  // in the following suggestions and body classes:
  //
  // page__node              page-node
  // page__node__%           page-node-%
  // page__node__1           page-node-1
  // page__node__edit        page-node-edit

the documentation in theme_get_suggestions() needs to be updated

  [class*="page-node-add-"] .messages,
  .page-node-edit .messages {
    margin-top: 1em;
    margin-bottom: 1em;
  }

In node-add.css - Will this still work? I don't think so.

  /**
   * Disabling all links (except local fragment identifiers such as href="#frag")
   * in node previews to prevent users from leaving the page.
   */
  Drupal.behaviors.nodePreviewDestroyLinks = {
    attach: function (context) {
      var $preview = $(context).find('.page-node-preview').once('node-preview');
      if ($preview.length) {
        $preview.on('click.preview', 'a:not([href^=#], #edit-backlink, #toolbar-administration a)', function (e) {
          e.preventDefault();
        });
      }
    },
    detach: function (context, settings, trigger) {
      if (trigger === 'unload') {
        var $preview = $(context).find('.page-node-preview').removeOnce('node-preview');
        if ($preview.length) {
          $preview.off('click.preview');
        }
      }
    }
  };

In node.preview.js this code now might be redundant if the node-preview class no longer exists - I think we need a follow to remove it

.page-admin-structure-block-demo

is in bartik's style css - I don't think this will work any more.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

I'll try to cover @alexpott suggestions :)

lauriii’s picture

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,618 pass(es). View
2.51 KB

I think I fixed some @alexpott suggestions. Let me know if I should do something else :)

Also created a followup for node-preview class stuff: https://www.drupal.org/node/2341995

rteijeiro’s picture

Assigned: rteijeiro » Unassigned
lauriii’s picture

FileSize
7.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,706 pass(es). View
1.62 KB

.page-admin class is still here so I brought back styles attached to that class. I also fixed block demo mode by adding a class to the demo block so we can attach styles there instead of body class.

rteijeiro’s picture

Status: Needs review » Needs work
+++ b/core/themes/bartik/css/style.css
@@ -1795,6 +1795,35 @@ div.add-or-remove-shortcuts {
+.page-admin #content img {

Could we get rid of ids?

  1. +++ b/core/themes/bartik/css/style.css
    @@ -1795,6 +1795,35 @@ div.add-or-remove-shortcuts {
    +.page-admin #content .simpletest-image img {
    

    This too.

  2. +++ b/core/themes/bartik/css/style.css
    @@ -1795,6 +1795,35 @@ div.add-or-remove-shortcuts {
    +.page-admin #admin-dblog img {
    

    This too.

  3. +++ b/core/themes/bartik/css/style.css
    @@ -1795,6 +1795,35 @@ div.add-or-remove-shortcuts {
    +#featured .demo-block {
    

    This.

  4. +++ b/core/themes/bartik/css/style.css
    @@ -1795,6 +1795,35 @@ div.add-or-remove-shortcuts {
    +#header .demo-block {
    

    And this.

lauriii’s picture

Status: Needs work » Needs review

I'm not sure if I was clear enough in my comment but .page-admin still exists so there shouldn't be need to remove these lines. Putting back to review!

rteijeiro’s picture

Status: Needs review » Needs work

Ok, please read carefully my suggestion. We should try to follow CSS coding standards so we should clean-up that code and try to avoid using ids. Could we use classes instead? Pretty pleaaaaase?

lauriii’s picture

Status: Needs work » Needs review

I actually first miss read your comment at first but the situation is that we cannot optimize the selectors anymore since there is now classes available for these elements. Putting back to RTBC. Lets get this in and party o/

mikl’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed #111. Wonderful to get rid of some of those useless classes, great job.

Tests pass and everything, lets get it in :D

tim.plunkett’s picture

Suggested retitle:
Remove useful body classes that shortsighted people assume no one will need

mortendk’s picture

@tim - which classes is that you are referring to i guess its the "html" class ;)

tim.plunkett’s picture

+++ b/core/includes/theme.inc
@@ -1677,27 +1677,29 @@ function template_preprocess_html(&$variables) {
-  $path_args = explode('/', current_path());
-  // Populate the body classes.
-  if ($suggestions = theme_get_suggestions($path_args, 'page', '-')) {
-    foreach ($suggestions as $suggestion) {
-      if ($suggestion != 'page-front') {
-        // Add current suggestion to page classes to make it possible to theme
-        // the page depending on the current page type (e.g. node, admin, user,
-        // etc.) as well as more specific data like node-12 or node-edit.
-        $variables['attributes']['class'][] = drupal_html_class($suggestion);
-      }
-    }

@@ -1787,15 +1789,6 @@ function template_preprocess_page(&$variables) {
-  // Set up layout variable.
-  $variables['layout'] = 'none';
-  if (!empty($variables['page']['sidebar_first'])) {
-    $variables['layout'] = 'first';
-  }
-  if (!empty($variables['page']['sidebar_second'])) {
-    $variables['layout'] = ($variables['layout'] == 'first') ? 'both' : 'second';

@@ -1887,16 +1880,16 @@ function template_preprocess_page(&$variables) {
-  // in the following suggestions and body classes:
+  // in the following suggestions:
   //
-  // page__node              page-node
-  // page__node__%           page-node-%
-  // page__node__1           page-node-1
-  // page__node__edit        page-node-edit
+  // page__node
+  // page__node__%
+  // page__node__1
+  // page__node__edit

This.

mikl’s picture

That's a somewhat misleading subset of the diff. The path-based suggestions are still there, just implemented differently.

mortendk’s picture

sidebar first & sidebar last classes is only relevant for a theme that is a 3 column layout + its regions is called "sidebar_first" and "sidebar_last"
bartik have both of those classes - it might be shortsighted but im pretty sure (ill bet you a beer & a pretzel) that the trend 2010 trend "3 column layouts" will change a bit the next couple of years when that responsive thing catches on ;)

rteijeiro’s picture

FileSize
205.48 KB

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

Sorry all, but this seems to be a css regression if we are just removing admin specific theming.

If this is to happen there needs to be alternative admin theming to account for the change.

+++ b/core/themes/bartik/css/style.css
@@ -1813,15 +1817,12 @@ div.add-or-remove-shortcuts {
-.page-admin #admin-dblog img {
-  margin: 0 5px;
-}

+++ b/core/themes/seven/css/layout/node-add.css
@@ -10,12 +10,6 @@
-
-  [class*="page-node-add-"] .messages,
-  .page-node-edit .messages {
-    margin-top: 1em;
-    margin-bottom: 1em;
-  }

Can you please provide before/after screenshots?

@tim.plunkett I think that "sort-sighted" comment may need a bit of explanation why that is the case.
It could very well be but telling a blind person they are blind doesn't help them see.

EDIT: had wrong diff but same message:

lauriii’s picture

.page-admin class is still there so theming admin pages specificly should be possible. What we have removed is possibility to theme specific pages.

LewisNyman’s picture

Assigned: Unassigned » LewisNyman

If this is to happen there needs to be alternative admin theming to account for the change.

I can fix this by adding the classes into the Seven theme.

LewisNyman’s picture

Assigned: LewisNyman » Unassigned
Status: Needs work » Needs review
FileSize
1.51 KB
8.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,767 pass(es). View

Ok, I've added a cleaner class to node add/edit pages in Seven. Much better than wild carding classes! It could do with a php review though...

+++ b/core/themes/bartik/css/style.css
@@ -1813,15 +1817,12 @@ div.add-or-remove-shortcuts {
-.page-admin #admin-dblog img {
-  margin: 0 5px;
-}

This didn't get removed, it just shifted up the file to fit better.

Suggested retitle:
Remove useful body classes that shortsighted people assume no one will need

The intention of the dreammarkup initiative has always been to serve default classes that most people will need, instead of trying to catch all the edge cases, otherwise we end up added a load of classes just in case they are needed by someone. It's ok to let people add their own classes and the banana initiative makes it a lot easier to control classes in the theme layer. Yay to less PHP!

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Patch #126 fixes the issue described on #123. Lets stop this bikeshedding.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
.page-admin #content img {
  margin-right: 15px; /* LTR */
}
.page-admin #content .simpletest-image img {
  margin: 0;
}
.page-admin #admin-dblog img {
  margin: 0 5px;
}

Are these not all .path-admin now? To test this I guess bartik needs to be the admin theme

mortendk’s picture

Status: Needs work » Needs review
Issue tags: -FUDK
FileSize
8.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,882 pass(es). View
525 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-126-129.patch. Unable to apply patch. See the log in the details link for more information. View

good catch renamed the classes to the correct path-admin

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Putting back to RTBC, fixes issue pointed by alexpott. To test this Bartik needs to be the admin theme.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 129: interdiff-126-129.patch, failed testing.

mikl’s picture

Status: Needs work » Reviewed & tested by the community

Shaddap, stupid test bot (interdiffs should not be tested).

catch’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1677,28 +1677,30 @@ function template_preprocess_html(&$variables) {
    +  $path = \Drupal::request()->getPathInfo();
    
    +++ b/core/themes/seven/seven.theme
    @@ -232,3 +232,16 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
    +  $path_args = explode('/', current_path());
    

    Could we also use Drupal::request->getPathInfo() here?

Also

+++ b/core/includes/theme.inc
@@ -1787,15 +1789,6 @@ function template_preprocess_page(&$variables) {
-    $variables['layout'] = ($variables['layout'] == 'first') ? 'both' : 'second';

Is this definitely no longer needed? Great if so.

lauriii’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.12 KB
8.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,406 pass(es). View

I dont see any functionality existing on Drupal core for #133.2. Fixed #133.1 in my patch

tuutti’s picture

Status: Needs review » Reviewed & tested by the community

Couldn't find any page.tpl using layout variable. Putting back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK. Committed/pushed to 8.0.x, thanks!

  • catch committed 9d5d3b3 on 8.0.x
    Issue #2234331 by mortendk, lauriii, LewisNyman, sqndr, rteijeiro, G-...
mortendk’s picture

Woooooohooooo ! :)
Highfives all around

Wim Leers’s picture

The CR was never published. I just did.

Status: Fixed » Closed (fixed)

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

zJoriz’s picture

Wow, you guys put a lot of work in this.
That said, does nobody want body classes for seeing if there's a View on deck, and if yes, which one? Or can I arrange that some other way (or did I simply miss the point here... distinct possibility)?

In most (if not all) of the sites I build, Views layouts differ quite a bit from Node layouts, usually starting at h1. Arguably I could set the classes for the Views as default and then make an exception for Nodes/Users/Taxonomy (since they can be detected).