I think the following...

{% set sidebar_first = page.sidebar_first|render %}
  {% set sidebar_second = page.sidebar_second|render %}
  <body{{ attributes.addClass(classes,
    not is_front ? 'with-subnav',
    sidebar_first ? 'sidebar-first',
    sidebar_second ? 'sidebar-second',
    (sidebar_first and not sidebar_second) or (sidebar_second and not sidebar_first) ? 'one-sidebar',
    (sidebar_first and sidebar_second) ? 'two-sidebars',
    (not sidebar_first and not sidebar_second) ? 'no-sidebar'
  ) }}>

Can be simplified to...

{% set classes = [] %}
{% for role in user.roles %}
  {% set classes = classes|merge(['role--' ~ role|clean_class]) %}
{% endfor %}
<!DOCTYPE html>
{% if ie_enabled_versions.ie8 %}
  {{- attach_library('basic/ie8') }}
{% endif %}
{% if ie_enabled_versions.ie9 or ie_enabled_versions.ie8 %}
  <!--[if lt IE 7]>     <html{{ html_attributes.addClass('no-js', 'lt-ie9', 'lt-ie8', 'lt-ie7') }}><![endif]-->
  <!--[if IE 7]>        <html{{ html_attributes.removeClass('lt-ie7') }}><![endif]-->
  <!--[if IE 8]>        <html{{ html_attributes.removeClass('lt-ie8') }}><![endif]-->
  <!--[if gt IE 8]><!--><html{{ html_attributes.removeClass('lt-ie9') }}><!--<![endif]-->
{% else -%}
  <html{{ html_attributes }}>
{% endif %}
  <head>
    <head-placeholder token="{{ placeholder_token }}">
    <title>{{ head_title|safe_join(' | ') }}</title>
    <css-placeholder token="{{ placeholder_token }}">
    <js-placeholder token="{{ placeholder_token }}">
  </head>
  <body{{ attributes.addClass(classes) }}>
    <div id="skip">
      <a href="#main-menu" class="visually-hidden focusable skip-link">
        {{ 'Skip to main navigation'|t }}
      </a>
    </div>
    {{ page_top }}
    {{ page }}
    {{ page_bottom }}
    <js-bottom-placeholder token="{{ placeholder_token }}">
    {% if browser_sync.enabled %}
      <script id="__bs_script__">
      document.write("<script async src='http://{{ browser_sync.host }}:{{ browser_sync.port }}/browser-sync/browser-sync-client.js'><\/script>".replace("HOST", location.hostname));
      </script>
    {% endif %}
  </body>
</html>

...by placing the following under Hook_preprocess_html

/*************************************************************
 * Prepares variables for the html.html.twig template.
 * @param $variables
 */
function basic_preprocess_html(&$variables) {
  try {
    $variables['is_front'] = \Drupal::service('path.matcher')->isFrontPage();
  }
  catch (Exception $e) {
    // If the database is not yet available, set default values for these
    // variables.
    $variables['is_front'] = FALSE;
  }

  if(!$variables['is_front']) {
    $variables['attributes']['class'][] = 'with-subnav';
  }

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

  if (!empty($variables['page']['featured_top'])) {
    $variables['attributes']['class'][] = 'has-featured-top';
  }

  // If we're not on the front page.
  if (!$variables['is_front']) {
    // Add unique classes for each page and website section.
    $path = \Drupal::service('path.current')->getPath();
    $alias = \Drupal::service('path.alias_manager')->getAliasByPath($path);
    $alias = trim($alias, '/');
    $alias = str_replace('/', '-', $alias);
    if (!empty($alias)) {
      $variables['attributes']['class'][] = 'page-' . $alias;
      list($section,) = explode('/', $alias, 2);
      if (!empty($section)) {
        $variables['attributes']['class'][] = 'section-' . $section;
      }
    }
  }

  // Add cachability metadata.
  $theme_name = \Drupal::theme()->getActiveTheme()->getName();
  $theme_settings = \Drupal::config($theme_name . '.settings');
  CacheableMetadata::createFromRenderArray($variables)
    ->addCacheableDependency($theme_settings)
    ->applyTo($variables);
  // Union all theme setting variables to the html.html.twig template.
  $variables += $theme_settings->getOriginal();
}
CommentFileSizeAuthor
#5 sidebar-layout-detection.patch2.56 KBanthony.bouch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

blue_waters created an issue. See original summary.

anthony.bouch’s picture

Issue summary: View changes
joelpittet’s picture

@blue_waters I'm having a hard time weeding through the example in the issue summary to see how it's simplified. Would you mind focusing the change?

Also we are following core's pattern of letting designers change classes in the template so that front-end devs don't need to know hooks and PHP to markup the pages so that's why classes are kept in the template for better or for worse.

anthony.bouch’s picture

Hi @joelpittet

If you have time, take a quick look at this gist...

https://gist.github.com/58bits/66968149924f80731c53

Scroll down to the html.html.twig file (and ignore the .php extension and <?php at the begining of basic.theme - they were needed to force Github to highlight the file).

You'll see that all of the logic to determine whether there are one, two, or no sidebars is now in function basic_preprocess_html(&$variables), and that html.html.twig is much cleaner and easier to read - take a look at the <body{{ attributes.addClass(classes) }}>, and compare that to what is currently in basic's html.html.twig file.

Doing it this way means you also don't have to call render on the sidebars either, and so the code below can be removed.

{% set sidebar_first = page.sidebar_first|render %}
  {% set sidebar_second = page.sidebar_second|render %}

This is also how the core classy theme does sidebar and layout detection.

So I would argue that for a designer, it might actually be a little easier to see what's going on inside html.html.twig this way.

Hope this helps and thanks again for all the work that was put into getting basic over to Drupal 8

anthony.bouch’s picture

Here's a patch for the 'classy'-style sidebar and layout detection options. I've tested this on a clean Basic install, with first, second and both sidebars present (it's also being used in production this way).

Hope this helps...

leahtard’s picture

Hey all!

I have confirmed the patch works. The classes get properly applied to the body for sidebar-first, sidebar-second or two-sidebars.

Throughout development, our goal was to keep basic.theme as empty as possible. It adds a layer of abstraction for those unfamiliar with Drupal's hook system. Much like what Joel was saying -- let's give frontend dev's the ability to customize these classes. I think the attributes.addClass array is a nice way to do that.

Cheers, Leah

leahtard’s picture

Status: Active » Closed (works as designed)

I'm going to mark this case as "works as designed" but please feel free to reopen if you feel there are additional arguments/benefits to implementing this alternative approach.

Cheers, Leah