Change the body classes to follow Drupal 8 CSS standards

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

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

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

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 ;)

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

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!

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?

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

.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 :-)

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.

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.

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.

+++ 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

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 {

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

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

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.

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

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 :)

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.

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.

+++ 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.

Let's figure this out in Austin

just reviewing now :)

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

Awesome in which case this patch works as intended ;)

+++ 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.

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

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

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

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

Freeing this up for someone else...

Reroll of #69.

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

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?

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
   

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

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.

Ups.. Changed it back to an array :)

I'm going to review this.

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

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.

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.

I fixed my own remark from #85.

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)

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

I'm going to add a change record.

Per #89.

I have created a change record for this.

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

Fixed issue summary.

Reviewed change record and seems good.

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

Is it RTBC now?

This looks good to me now as well.

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.

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.

Updating the issue summary.

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

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

Minor reroll in light of #2321505: Split Seven's style.css into SMACSS categories

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

Actually back to RTBC.

  // 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.

I'll try to cover @alexpott suggestions :)

Created issue for #98: #2341983: Remove unused variable from template_preprocess_page()

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

.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.

+++ 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.

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!

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?

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/

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

Tests pass and everything, lets get it in :D

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

+++ 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.

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

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:

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

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.

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!

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

.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

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

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

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.

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

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

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

The CR was never published. I just did.

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).