Follow up from #2376147: Installer is missing all of the global Seven theme stylesheets 

function seven_preprocess_html(&$variables) {
  // Add information about the number of sidebars.
  if (!empty($variables['page']['sidebar_first'])) {
    $variables['attributes']['class'][] = 'one-sidebar';
    $variables['attributes']['class'][] = 'sidebar-first';
  }
  else {
    $variables['attributes']['class'][] = 'no-sidebars';
  }

The classes this adds are completely unused. Dead code.

CommentFileSizeAuthor
#1 2377393.1.patch818 bytesalexpott

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2377393.1.patch818 bytes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -D8MI, -language-base, -sprint
sqndr’s picture

sqndr commented
Status: Needs review » Needs work

Yep, that's true. These classes can be removed. While we're working in the preprocess function, it seems like the code below contains some more issues:

  // If on a node add or edit page, add a node-layout class.
  $path_args = explode('/', \Drupal::request()->getPathInfo());
  if ($suggestions = theme_get_suggestions($path_args, 'page', '-')) {
    foreach ($suggestions as $suggestion) {
      if ($suggestion === 'page-node-edit' || strpos($suggestion, 'page-node-add') !== FALSE) {
        $variables['attributes']['class'][] = drupal_html_class('node-form-layout');
      }
    }
  }
  1. The comments says it's adding the node-layout, while the node-form-layoutclass is added.
  2. drupal_html_class() is deprecated: @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.0.
  3. I was even wondering if we still need that last part? Can't we copy the node-edit-form.html.twig template form the node module and add the class there?
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

@sqndr this issue is only about removing unused classes - your recommendations look sound but out of scope.

sqndr’s picture

sqndr commented

The first two issues I listed are, but the third question was do we need that last part? The scope of the issue seems to be removing 'unused classes'; and it seems like not all the 'unused classes' are removed in the issue.

Setting it back to 'Needs review' to see what testbot has to say. :)

As discussed with @alexpott - I'll create a new issue for removing the last part 'cause it might need some more work. This keep the issue scope nice and small.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +dreammarkup

Nice cleanup! I double checked and there are no uses in seven and even bartik has converted most to .layout-sidebar-first/no-sidebars/on-sidebar.

@sqndr I think we are "re-imagining" that hunk here: #2329753: Move html classes from preprocess to templates

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

RTBC++

  • webchick committed c2cde8f on 8.0.x 
    Issue #2377393 by alexpott: Seven seven_preprocess_html adds unused...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Nice find. Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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