Use branding block in place of page template branding variables (site name, slogan, site logo)

Should we add a "header-left" region and rename "header" to "header-right"? We could then put the new blocks in "header-left" and the old header region would be left unchanged for the most part.

If you split header, it should be header_first and header_second, not left/right, to work best with RTL.

Here is an updated patch which is almost done.
Some things to note:
1. it will not work without the patch from http://drupal.org/node/1053648#comment-7465426 already commited to the branch.
2. it is a work in progress.
3. It needs to be tested in IE and in a RTL language.

Ok, fail. New patch.

Can we get some before and after?

Updating status.

Can you decouple this from the other issue? This would most likely need to go in first.

use # link

So? If nothing is in it, nothing should change. This should just add a place for those blocks to eventually go.

reorganizing

Right, but all of this is optional. If I disable the blocks, nothing should look broken.

Even if the other issue went in first, we'd still want screenshots of how it looks with no blocks in the new region.

Ok, here is a new patch and a bunch of pictures(there are a lot of pictures).

For reference, I have screenshots of D7 and D8 before the patch is applied:

Here is with the patch and with the blocks placed:

Here are screenshots with no blocks in either header region:

One thing to note is that having long element, or lots of blocks in the header regions can mess up the layout:

Also, the layout is actually broken in RTL, but that is because most of Bartik is broken in RTL. I made it look as good as I could, but further work on that should be done in an issue specifically to fix Bartik in RTL.

I tested on my Galaxy S2, and it looks ok in both landscape and portrait mode, but we should test it on some other devices also.

It is broken in IE8 as it does not support media queries.

Finally, I tested installing Drupal with this patch, but not the patch that creates the blocks, applied and the installer fails in that scenario.

Clarify the scope of this issue.

Due to changes in the approach in #1053648: Convert site elements (site name, slogan, site logo) into blocks this patch is likely to be significantly simplified, keep an eye on that issue. This could be postponed, however momentum is clearly behind the new approach, looks like its getting close.

This patch will definitely fail, but it's a rewrite of the above work given the changes that have been done in #1053648: Convert site elements (site name, slogan, site logo) into blocks to create a site branding block.

Since that part is already handled, the two things this patch does is:

1. Split the header region into two regions: header_first and header_second
2. Place a site branding block into header_first on install.

The only way this patch might work is if you install the patch from #1053648: Convert site elements (site name, slogan, site logo) into blocks first. Once that's committed, you can skip that step of course.

If you're using Bartik, you'll see your site branding disappear after installing this patch. You'd have to reinstall the site to see the branding by default. You could also manually add a branding block to header_first on the Block Layouts page.

I've been trying to keep the markup largely the same from what it is was before this patch in order to simplify testing. I think we certainly could do further cleanup of the markup in a followup Dreammarkup issue.

I did make a couple slight changes. Right now, Bartik will simply inherit branding.html.twig. That means the site branding elements will use classes rather than an id on each element. If there's a really really good reason to use an id for these elements, we could override this, but it seems unnecessary and unwise unless there's some JS that targets those elements, which seems unlikely.

The other thing that Bartik's markup did was to switch the wrapper for the site name to h1 if there was no other title on the page. I suppose we could add that back in, either as an override or in system's branding block. Since branding blocks can appear in any region, however, I think you'd want some sort of logic that an h1 should only be used if in the header_first region or something like that. You wouldn't want h1 elements littered all over the page.

The only other thing specific to Bartik's template was adding in a visually-hidden class to the branding elements if they were turned off on the global appearance or theme settings page. Personally I'd like to see those controls removed in a follow-up issue. Since the branding block allows the name, logo and slogan to be turned on and off, I think it doesn't make sense to override that from the appearance page. If the branding block has the name turned off, the block should honor that, not just hide it. I believe there was an accessibility reason for using visually-hidden, but I think that made a lot more sense when the name, logo and slogan were hard-coded into the template rather than showing up based on block controls.

For the record, I also think it's really wasteful for the CSS to duplicate the region-header-first, region-header-second styles for every single rule. I guess it boils down to whether we want those classes duplicated in every CSS declaration, or if we would want to add a region-header class to region-header-first and region-header-second, probably through a preprocess function in bartik.theme. I'm not a fan of either, so I went with duplicating the class decorations in the CSS for the sake of expediency.

Usually helps if I do git add for the new yml file that installs the branding block.

+++ b/core/themes/bartik/css/colors.css
@@ -50,11 +50,14 @@ a:active,
+.region-header-second,

Suggest using attribute selectors such as [class*="region-header-"] instead of duplicating selectors, we could as you say inject a common class, not sure if we need to or not at this stage, it might depend how #1869476: Convert global menus (primary links, secondary links) into blocks pans out.

Great idea, I like it!

One thing to mention based on the most recent patch:

+++ b/core/themes/bartik/templates/page.html.twig
@@ -82,45 +83,20 @@
+    {% if page.header_first %}
+      {{ page.header_first }}
     {% endif %}

Unless we're wrapping markup around page.header_first there's no need for the if tag in cases like this.

I'll keep that in mind!

Hoping to draft up a new patch for this tonight now that #1053648: Convert site elements (site name, slogan, site logo) into blocks is in.

This is primarily just a reroll, so that there's a clean starting point for future patches.

I'll be working on a new patch from here.

Here's the real meat and potatoes.

I used the suggestion from Jeff in #22 to simplify some of the CSS. I also have this working for site maintenance page as well as the regular page template, and for system/Stark as well as Bartik.

The other big thing in this patch is that I've done my best to remove the feature checkboxes for logo, name and slogan from the appearance Global Settings page as well as the theme settings page.

One really big thing to note in terms of the affects on existing themes already out there:

The variables for the site slogan, name and logo are still being sent to the page template variables, even if they aren't controlled by toggles to turn them on and off. That's probably good, as there could in theory be other reasons to use those variables. But more importantly, this should **not** break existing themes that use those variables. The logo, slogan and and name will still show up.

What will change is the sort of dev sites most core folk are using that probably have Bartik turned on by default. The slogan, logo and name will disappear from those installs when they pull down the latest dev version, unless they do a reinstall (which auto-installs a site branding block) or they add a site branding block. While that might be a surprising disappearance, I don't think that completely breaks those sites. And from my experience, I have to do reinstalls now and then anyhow.

The one thing I didn't tackle in this patch was tests. I'm guessing there are tests to check if the logo toggle feature works, for example. My guess is that if those tests exist, they will break. But that's good, as it helps me to find those tests!

Anyhow, good chance this will take a few tries to get into decent shape. Feedback welcome!

Here are before and after screenshots. The branding elements look visually identical from when variables were in the template to when a site branding block is used.

Now I know what tests this breaks so I can fix those. Yay!

The errors with the system branding block test surprise me. Would a difference in markup for Bartik affect that or does it test system/Stark?

This should have fewer test failures. I fixed the tests that were based on placing blocks in a header region: changed those to header_second region which is the equivalent of the previous header region.

There are a few other test errors I might need assistance with.

There are some language errors that look like they're connected with the site slogan. I didn't think I changed the config names for site name and site slogan, but maybe something is going on there?

There are also some tests related to the h1 wrapped around the site name, which isn't in this new template.

Previously Bartik and I think Stark too tested if a page title was defined: if not, it wrapped the site name in an h1. We discussed this on a Twig call and decided that wasn't needed. It would be very difficult to reproduce: you wouldn't want every site branding block to have an h1, and blocks are no longer aware of their regions (as far as I know), so I don't think we could just say use an h1 if the block is in the header_first region. So that's kind of tricky.

Once some more tests pass, it will be easier to see what's still broken.

Found a few more test bugs. Some will probably still pop up.

There is lots of nice cleanups in that. Good work, here's a reviews on some nitpickery and a few questions and duplicates.

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -262,8 +262,9 @@ function testBlockRehash() {
   +    $block = $this->drupalPlaceBlock('test_cache', array('region' => 'header_second'));
   +    $block = $this->drupalPlaceBlock('test_cache', array('region' => 'header_second'));
   

   Duplicate?

2. +++ b/core/themes/bartik/config/block.block.bartik.branding.yml
   @@ -0,0 +1,21 @@
   +  label_display: '0'
   

   Same question about the string vs int yaml values.

3. +++ b/core/themes/bartik/css/layout.css
   @@ -24,12 +24,20 @@ body,
   +    margin: .5em 5px .75em;
   +    border: 1px solid #ccc;
   

   two space tabs here.

4. +++ b/core/themes/bartik/css/maintenance-page.css
   @@ -31,16 +31,16 @@ body.maintenance-page {
   +.maintenance-page .site-branding-text a:hover,
   +.maintenance-page .site-branding-text a:hover {
   

   This is a duplicate.

5. +++ b/core/themes/bartik/css/style.css
   @@ -420,89 +412,89 @@ h1.site-name {
   +[class*="region-header-"] .block-menu li a {
   

   This type of selector looks messy but maybe it was recommended? Just my opinion.

6. +++ b/core/themes/bartik/css/style.css
   @@ -1815,29 +1801,25 @@ div.admin-panel .description {
   +  [dir="rtl"] .site-branding-text{
   

   Space missing at end.

7. +++ b/core/themes/bartik/templates/maintenance-page.html.twig
   @@ -27,22 +27,9 @@
   +
   ...
   +
   

   Probably don't need the extra new lines here.

8. +++ b/core/themes/stark/config/block.block.stark.branding.yml
   @@ -0,0 +1,21 @@
   +  label_display: '0'
   

   I don't know yaml that well but noticed you were changing other values. Should this be an int too?

Fixed the issue 1, 3, 4 and 6. Tried to fix extra lines in 7, although I'm not precisely sure which lines were the extra ones.

For items 2 and 8, I'm trying to match the block config file with the block config schema file. I used an example file for the block module, but I'm honestly not sure if I'm doing it right. Making corrections based on what Testbot says, and I found a couple more to correct. label_display says string in the schema file, but I'm honestly not sure why.

I think a lot of the test failures stem from the fact that the test is expecting site elements to be in the template. I think I need to add a default site branding block before the assertion. So I need to figure out how to do that next.

Nice work you shaved off 2 fails, and #7 was the lines I was refereing too, sorry needed more context.

--- /dev/null
+++ b/.idea/dictionaries/mdrummond.xml

+++ b/.idea/dictionaries/mdrummond.xml
@@ -0,0 +1,3 @@

@@ -0,0 +1,3 @@
+<component name="ProjectDictionaryState">
+  <dictionary name="mdrummond" />
+</component>
\ No newline at end of file

You should exclude .idea from your global ~/.gitignore or inside .git/info/exclude file.

This should fix the test fails in ThemeTest and LanguageUILanguageNegotiationTest. Needed to place a site branding block to make those work. Also removed the stupid .idea file that snuck in there.

Realized config files that installed site branding block were in the wrong place. Moved to standard profile. Looks like Stark doesn't need a separate config file in the minimal profile. We'll see if this works. Schema files not needed either.

Awesome!! It's showing up now on install and switching to stark, woot:)

The fails on the PageTitle test are due to these tests @ line 98:

      'toggle_name'           => TRUE,
      'toggle_slogan'         => TRUE,

And the ThemeTest.php exception is because it can't find it, which I think is because you are placing the block($this->drupalPlaceBlock('system_branding_block', array('region' => 'header_first'));) after you make the request for the page($this->drupalGet('');).

Hope that helps cull some errors.

Regardign selectors like [class*="region-header-"] .block-menu li a I'm not overly worried about what they look like, more that they work, and are not over-specified (which is something Bartik has always suffered from). I tend to use that form of attribute selector quite a lot mainly because it's reliable, which for me trumps other forms that may look more elegant.

@Jeff Burnz, I'm coming around to that syntax and my concern stems mostly from years of IE emotional abuse(Some call it experience...). Likely IE9+ has gotten rid of their bugs, though I've avoided that syntax due to previous IE bugs and knowing I could "get by" without it. So let's leave it in there and I'll just tag this for some ie testing at the end.

Keep up the great work @mdrummond!

More attempts at fixing the tests. Thanks for the review and the suggestions, joelpittet!

Here's a reroll, as there was a change to theme.inc. Posting this so the next interdiff is clean.

testbot engage.

This patch should fix the config translation UI test and the page title test. I still can't figure out the Block UI Test, though. Having troubles running tests on my local install (not sure why), so if anybody could help debug that test, it would be very much appreciated.

+++ b/core/themes/bartik/templates/maintenance-page.html.twig
@@ -27,22 +26,7 @@
+      {{ page.header_first }}

I didn't think this variable would be available in the maintenance page template, so I tested manually and took before & after screenshots to demonstrate:

Before

After

This should fix those test failures/exceptions. Adds the block module to Drupal\config_translation\Tests\ConfigTranslationUiTest and Drupal\system\Tests\System\PageTitleTest and fixes a silly xpath thing. xpath--

@mdrummond, not sure what issue you were having running the tests locally but wanted to mention that you should have twig_debug off when running tests.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
@@ -40,7 +40,7 @@ public function setUp() {
-    $this->drupalPlaceBlock('system_branding_block', array('region' => 'header', 'id' => 'site-branding'));
+    $this->drupalPlaceBlock('system_branding_block', array('region' => 'header_second', 'id' => 'site-branding'));

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php
@@ -144,6 +144,9 @@ public function testSiteInformationTranslationUi() {
+    // Place branding block with site name and slogan into header_first region.
+    $this->drupalPlaceBlock('system_branding_block', array('region' => 'header_first'));
+

+++ b/core/modules/language/lib/Drupal/language/Tests/LanguageUILanguageNegotiationTest.php
@@ -389,6 +389,9 @@ function testUrlLanguageFallback() {
+    // Place a site branding block in the header_first region.
+    $this->drupalPlaceBlock('system_branding_block', array('region' => 'header_first'));
+

+++ b/core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php
@@ -109,6 +107,9 @@ function testTitleXSS() {
+    // Place branding block with site name and slogan into header_first region.
+    $this->drupalPlaceBlock('system_branding_block', array('region' => 'header_first'));
+

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
@@ -114,9 +114,10 @@ function testThemeSettings() {
+      $this->drupalPlaceBlock('system_branding_block', array('region' => 'header_first'));
       $this->drupalGet('');

@@ -166,8 +167,9 @@ function testThemeSettings() {
+    $this->drupalPlaceBlock('system_branding_block', array('region' => 'header_first'));

I may be really confused, but since this patch actually places the block by default, do we need all the block placed code in here at all? Sorry for my ignorance.

Also, neutralizing @Cottser's xpath karma. Because without it we have to care about whitespace way too much.
xpath++
:P

Just a quick idea in order to let people still do hard work in templates. Write a twig extension which allows you to render an arbitrary block somewhere. These blocks would be also cachable...

I think it's time to revive this from the grave. Doing some similar work in #1869476: Convert global menus (primary links, secondary links) into blocks.

I haven't checked, but I will bet a lot of money we need a reroll on this one.

Anyone want to help with a reroll on this? Otherwise I'll try to look at this soon.

Yes, let's get this moving forward again. I'll provide reviews.

Please review it for something I did wrong :)
This patch will fail, there is a conflict that is not how to solve :(

<<<<<<< HEAD:core/modules/block/src/Tests/BlockTest.php
  /**
   * Tests that uninstalling a theme removes its block configuration.
   */
  public function testUninstallTheme() {
    /** @var \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler */
    $theme_handler = \Drupal::service('theme_handler');
=======
    // Add a test block.
    $block = array();
    $block['id'] = 'test_cache';
    $block['theme'] = \Drupal::config('system.theme')->get('default');
    $block['region'] = 'header_second';
    $block = $this->drupalPlaceBlock('test_cache', array('region' => 'header_second'));
>>>>>>> patching...:core/modules/block/lib/Drupal/block/Tests/BlockTest.php

Greetings...

from old file core/modules/block/lib/Drupal/block/Tests/BlockTest.php

Then let me try...

I think we will need new regions for #507488: Convert page elements (local tasks, actions) into blocks and #2289917: Convert "messages" page element into blocks, postpone or forge ahead and do new followups?

I don't think those issues are interdependent on each other. I believe in messages we were looking on taking care of the new region within that issue. I can't remember where we were at for title, tabs and actions. But I'd rather handle the regions separately for each of those, rather than making one mega-patch which is harder to review and has more dependencies.

@lokapujya yes I believe it should. The reroll may have caught some changes from #1869476: Convert global menus (primary links, secondary links) into blocks

style.css on Bartik is gone, which has been split into different files. So I've had to hunt down line by line every change and reroll that part manually, it probably could use some more eyes on that part to check that I didn't miss anything, and that we're still ok there.

Ive just ran a clean install with the attached patch applied, the block isn't showing up in its place, so that needs to be done. The site branding block is available to be added through the block layout ui in admin/structure/block. Once enabled it looks fine to me although I haven't done a one to one comparison.

In any case, find attached #87 rerolled =)

• Fixed the block not showing up on standard install.
• Added the block for minimal install profile.

Should this also be using the the "classy" tag?

I'm guessing yes.

Added config schema for the system_branding_block, this should get rid of some of the failing tests.

I've made the changes to the classy templates page.html.twig and maintenance-page.html.twig that we did to core's. This is something we should do sooner or later, and will get rid of some of the failing tests.

Don't think this is Bartik specific.

If you need to modify page template, make sure you get all of them. I don't check the whole patch, but there is also the install-page template, Seven has one, etc.

Attached a patch just adding missing comments for the new regions on classy page.html.twig (no need for rerunning the tests...)

Very true @davidhernandez ... I've taken a look, and both install-page.html.twig and maintenance-page.html.twig on seven have this in them:

  <header role="banner">
    {% if site_name %}
      <h1 class="page-title">{{ site_name }}</h1>
    {% endif %}
  </header>

So this begs the question... what to do about this? site_name with this patch is a part of a block that includes the logo, the site name and the slogan... any clues?

Well, you never ever put that code back that you removed ...

So there is nothing placing those things in the regions ...

-    {% if logo %}
-      <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">
-        <img src="{{ logo }}" alt="{{ 'Home'|t }}"/>
-      </a>
-    {% endif %}

I think I would leave the code for now and just add the new regions for usage later?

Or maybe a template_preprocess_region__header_one?

( you need to register that first )

@Fabianx I'm not sure I follow your comment (#102)...

Here is some more progress on this:

• Cleaned up a bit SystemBrandingBlock::build(), getting only the logo.url instead of the whole logo config, and sanitizing the site name, like we were doing on theme.inc.
• Removed the logo, site_name and site_slogan from theme.inc's template_preprocess_page(), removed associated comments on core's template files, and introduced just site_name into seven_preprocess_install_page and seven_preprocess_maintenance_page, where they are still necessary.

Let's see what the test bot thinks about this one...

This should fix a lot of them failing tests... I could use input on whether this is the right path, etc.

Also, I spent some time trying to fix LanguageUILanguageNegotiationTest, but I apparently I'm not the one to fix this one... any pointers?

1. +++ b/core/includes/theme.inc
   @@ -328,14 +328,12 @@ function theme_get_setting($setting_name, $theme = NULL) {
          // Generate the path to the logo image.
   -      if ($cache[$theme]->get('features.logo')) {
   -        $logo_path = $cache[$theme]->get('logo.path');
   -        if ($cache[$theme]->get('logo.use_default')) {
   -          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.svg'));
   -        }
   -        elseif ($logo_path) {
   -          $cache[$theme]->set('logo.url', file_create_url($logo_path));
   -        }
   +      $logo_path = $cache[$theme]->get('logo.path');
   +      if ($cache[$theme]->get('logo.use_default')) {
   +        $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.svg'));
   +      }
   +      elseif ($logo_path) {
   +        $cache[$theme]->set('logo.url', file_create_url($logo_path));
          }
   

   Why this change?

2. +++ b/core/modules/color/color.module
   @@ -112,7 +112,8 @@ function color_library_info_alter(&$libraries, $extension) {
    function color_preprocess_page(&$variables) {
      $theme_key = \Drupal::theme()->getActiveTheme()->getName();
   -
   +  $site_config = \Drupal::config('system.site');
   +  $variables['logo'] = $site_config->get('logo');
      // Override logo.
      $logo = \Drupal::config('color.theme.' . $theme_key)->get('logo');
      if ($logo && $variables['logo'] && preg_match('!' . $theme_key . '/logo.svg$!', $variables['logo'])) {
   

   I don't think this is the right change?

   color_preprocess_page() in HEAD overwrites what template_preprocess_page() set as the value for $variables['logo'].

   But… we're modifying template_preprocess_pager() to no longer set that. So it's pointless for the color module to overwrite it, because it won't be output anyway.

   If we want to keep color module modifying the logo, then we should make it alter the block that contains the logo.

3. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
   @@ -165,16 +166,16 @@ public function build() {
   -    $logo = theme_get_setting('logo');
   +    $logo_url = theme_get_setting('logo.url');
        $build['site_logo'] = array(
          '#theme' => 'image',
   -      '#uri' => $logo['url'],
   +      '#uri' => $logo_url,
   

   Cleaner & simpler, good :)

   Let's make it even simpler:

   '#uri' => theme_get_setting('logo.url'),
   

4. +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -115,9 +115,10 @@ function testThemeSettings() {
   +      // Verify the actual 'src' attribute of the logo being output in a site branding block.
   

   80 cols.

5. +++ b/core/modules/system/templates/page.html.twig
   @@ -63,33 +63,9 @@
   +    {{ page.header_first }}
    
   -    {{ page.header }}
   +    {{ page.header_second }}
   

   If we're putting these regions next to each other… then why bother having two of these regions?

6. +++ b/core/profiles/minimal/config/install/block.block.stark_branding.yml
   @@ -0,0 +1,18 @@
   +  label: 'Page Site Branding'
   
   +++ b/core/profiles/standard/config/install/block.block.bartik_branding.yml
   @@ -0,0 +1,18 @@
   +  label: 'Page Site Branding'
   

   Let's omit the "Page" — just "Site Branding" is sufficient, isn't it?

7. +++ b/core/themes/bartik/css/colors.css
   @@ -43,12 +43,12 @@ a:active,
   +[class*="region-header-"],
   +[class*="region-header-"] a,
   +[class*="region-header-"] li a.active,
   

   Woah, what? Do we ever do this? You're using a clever selector to match both .region-header-first and .region-header-second in one go.

   But not only does that look odd/confusing, it also means we match .region-header-foobarbaz, plus it's very likely also bad for performance. Let's be explicit?

   (Also, if it turns out a single header region is sufficient, these changes can even be reverted.)

8. +++ b/core/themes/bartik/templates/maintenance-page.html.twig
   @@ -12,22 +12,7 @@
        <header id="header" role="banner">
   ...
   +        {{ page.header_first }}
   

   Another reason for not having two header regions: the cognitive dissonance between #header containing "header first", and "header second" not appearing anywhere in this template…

9. +++ b/core/themes/bartik/templates/page.html.twig
   @@ -77,35 +67,8 @@
   +        {{ page.header_first }}
   +        {{ page.header_second }}
   
   +++ b/core/themes/classy/templates/layout/maintenance-page.html.twig
   @@ -14,26 +14,8 @@
   +    {{ page.header_first }}
   +    {{ page.header_second }}
   
   +++ b/core/themes/classy/templates/layout/page.html.twig
   @@ -62,32 +59,8 @@
   +    {{ page.header_first }}
   +    {{ page.header_second }}
   

   Here they are adjacent too.

In conclusion, it looks like the main reason for having two header regions is that Bartik currently floats the header to the right, but you need some things (logo + site name + slogan) to float to the left. But everywhere, we have both header regions adjacent. Which makes me think that what we really should change is Bartik's CSS. Or… use "header" everywhere, but in Bartik add "left header" and "right header".

Of course, I'm no themer, so I defer to the collective themers' thinking to make a decision here.

just to chime in quickly.

Using * selectors in css is maybe not always the way we wanna go, unless theres a really really good reason for it ;)
use .region-header-name, .region-header-othername, .region-header-yetanothername { ... } instead.

Thanks @Wim Leers for the review! My answers to your points on #107:
1. I don't know, was part of the reroll... perhaps someone else knows the answer?
2. You're right. And actually the logo is just now a transparent svg so removing the preprocess all together, I don't believe we need to be altering the logo any longer? I've tested it without it and it looks fine on different colors. The attached patch removes this function.
3. True, done.
4. Done.
5. Again part of the reroll.
6. Done.
7. I agree, they're stil there because of the reroll. There are a lot more like this on core/themes/bartik/css/components/header.css. We should clean these up.
8. Yes.
9. Again, part of the reroll.

As for the regions we are introducing, as it stands now in the patch, these are not my call, and I do agree they don't make much sense at the moment. Having a header_first, header_second, and then just plain header is confusing.

I'm not sure whether this was discussed earlier on, if there is a consensus on it or whether we should do this now.

I see two regions, like you said, left and right. Although in today's world we named them first and second rather than by their desktop layout. I think the Bartik team should chip in on this so we can move this patch forward, without causing a mess...

Once we've cleared out how to deal with the header regions, we'll tackle cleaning up the css selectors that are in the patch.

On another front, only one failing test w00t!

#109.1: let's remove that hunk then and see what happens?

RE: header_first/header_second: looks like this is indeed coming from a long time ago. Pinging @mdrummond to review this.

OK, I've done some more digging about #109.1, and it makes sense, since the patch removes 'logo' as a default feature here on ThemeHandler:

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -29,10 +29,7 @@ class ThemeHandler implements ThemeHandlerInterface {
-    'logo',

So unless we remove that check, the logo would never appear...

Yes, header first and and header second is so we can handle the layout in Bartik, so it's fine if those are specific to Bartik.

Using header first and header second is preferable to header left and header right because on smaller viewports, there's a good chance those aren't floated to the left and right. That's the same naming pattern used elsewhere (at least from my memory).

Yes, header first and and header second is so we can handle the layout in Bartik, so it's fine if those are specific to Bartik.

Ok, so let's do that. If you could do that, @Manuel Garcia, or @mdrummond, I'd be happy to provide reviews!

Using header first and header second is preferable to header left and header right because on smaller viewports […]

Absolutely!

I also think this is at major since it affects the TX significantly.

#2398451: Clean up "layout" CSS in Bartik landed... trying to reroll this.

Can't do this at the moment, unassigning myself until I find some quite time...

I may have some time available to work on core again, so I'll see if I can familiarize myself with this again. This at the top of my list of the things I want to see get into core.

Bumping to major as per Wim's comment.

#117: YAY! :) I'll support you in any way I can: patch reviews in this issue, but also being the person to bounce ideas off if you need that :)

Patch rerolled

Fixed some bugs.

Sorry. Uploaded wrong patch.

Thanks for the rerolls & bugfixes!

The next step here is to remove the distinction between header_first and header_second, and only have header. Just like HEAD. That also means less change in this patch.

See #107, #110, #113.

1. +++ b/core/themes/bartik/css/components/header.css
   @@ -107,33 +93,37 @@ h1.site-name {
   +[class*="region-header-"] .block .content {
   ...
   +[class*="region-header-"] .block ul {
   ...
   +[class*="region-header-"] .block li {
   

   Instead of using this selector, we should add a common class that these divs share. That would be closer to our CSS standards

2. +++ b/core/themes/bartik/css/components/header.css
   index bb23434..069b76c 100644
   --- a/core/themes/bartik/css/layout.css
   
   --- a/core/themes/bartik/css/layout.css
   +++ b/core/themes/bartik/css/layout.css
   
   +++ b/core/themes/bartik/css/layout.css
   +++ b/core/themes/bartik/css/layout.css
   @@ -11,6 +11,135 @@
   

   I think this CSS should go in the header.css and other relevant files. We are trying to move towards having all the CSS for one component in the same file. See: #2398451: Clean up "layout" CSS in Bartik

3. +++ b/core/themes/bartik/css/layout.css
   @@ -11,6 +11,135 @@
   +.region-header-second {
   +  margin: .5em 5px .75em;
   ...
   +  .region-header {
   +    margin: .5em 5px .75em;
   

   We should always include the leading 0 when writing decimal units.

First a reroll.

https://www.drupal.org/contributor-tasks/update-allowed-beta

Firing testbot to test the patch in #128.

Here's a re-roll again from #125

Working on removing the header_left and header_right regions as mentioned a few times by @Wim Leers above.

This should fix the errors (mostly related to String -> SafeMarkup).

Also removes most of the CSS that looks like it got re-rolled in by accident (may need more CSS). Yet most of it's not needed anyways.

+++ b/core/modules/system/templates/page.html.twig
@@ -63,32 +63,6 @@
-    {% if logo %}
-      <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">
-        <img src="{{ logo }}" alt="{{ 'Home'|t }}"/>
-      </a>
-    {% endif %}
-
-    {% if site_name or site_slogan %}
-      <div class="name-and-slogan">
-
-        {# Use h1 when the content title is empty #}
-        {% if title %}
-          <strong class="site-name">
-            <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
-          </strong>
-        {% else %}
-          <h1 class="site-name">
-            <a href="{{ front_page }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
-          </h1>
-        {% endif %}
-
-        {% if site_slogan %}
-          <div class="site-slogan">{{ site_slogan }}</div>
-        {% endif %}
-      </div>{# ./name-and-slogan #}
-    {% endif %}
-

:D!

This should resolve the last few test failures... small possibility it may make things worse. Let's roll the dice;)

This fixes a bizarre broken test and the need for that cache tag addition in the test because we aren't using absolute URLs for the branding front links.

+++ b/core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php
@@ -401,13 +404,17 @@ function testUrlLanguageFallback() {
-    $this->assertTrue($fields[0] == 'Drupal', 'URLs are rewritten using the browser language.');
...
+    $this->assertTrue(!empty($fields) == 'Drupal', 'URLs are rewritten using the browser language.');

Weird!

There is still some regression on the visuals

lauriii++

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   --- a/core/modules/color/color.module
   +++ b/core/modules/color/color.module
   

   Hrm, I don't see how this functionality is being retained?

2. +++ b/core/modules/system/templates/block--system-branding-block.html.twig
   @@ -17,13 +17,13 @@
   -    <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home">
   +    <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">
   

   This is a good change; the branding block in HEAD is wrong in this way; it should've been using path() all along.

3. +++ b/core/themes/seven/seven.theme
   @@ -146,16 +146,20 @@ function seven_element_info_alter(&$type) {
   +  $variables['site_name'] = $site_config->get('name');
   ...
   +  $variables['site_name'] = $site_config->get('name');
   

   Great, but now these docs in (install|maintenance)-page.html.twig are outdated:

    * All available variables are mirrored in page.html.twig.
    * Some may be blank but they are provided for consistency.
   
   

For some reason the first 4 steps of the installer have lost the Drupal logo when you apply the patch in #138.

Missing the Drupal page title

It's back!!!!

Also as Classy and Stark do not set blocks, we have lost the 'Site Branding' showing on install in those themes. What should we do about this if anything? If we lose the branding we will need to update the screenshots for those themes as part of this issue.
 

Some of the regression is because Stark and Classy automatically had site name in the default page template as a variable. To not have a visual regression, we'd have to treat Stark and/or Classy like real themes and add a default block configuration so they are there by default when installed. Technically, we don't have to do that, if we don't consider them necessary parts of the "product".

Tagging for product manager feedback, to see if it is ok to leave it out or not.

This doesn't fix everything but should fix the visual regressions from @lauriii in #139

    --- a/core/modules/color/color.module
    +++ b/core/modules/color/color.module

Need to fix fix this, likely with a hook_block_view_BASE_BLOCK_ID_alter or something

    +++ b/core/themes/seven/seven.theme
    @@ -146,16 +146,20 @@ function seven_element_info_alter(&$type) {
    +  $variables['site_name'] = $site_config->get('name');

    Great, but now these docs in (install|maintenance)-page.html.twig are outdated:
     * All available variables are mirrored in page.html.twig.
     * Some may be blank but they are provided for consistency.

This may be related to what @emma.maria was seeing in the installer.

The logo still is not appearing on the installer, as evidence by the attached screenshot. It appears by inspecting markup that the "page-title" element is missing entirely from the output.

Sounds like it needs work

@Wim Leers helped with the color module issue, it was quite broken in head, this should make it kinda work.

I also opened this #2488364: hook_block_view_alter doesn't get the $build array from the build() function because of trying to fix that issue.

Also this should fix the broken maintenance/install pages, went a bit overboard and didn't need to convert them.

Found this issue while testing things yesterday #2259567: Support SVG files for theme logo setting

I discussed this a bit with webchick. Not having Stark and Classy set blocks by default is fine. Just make sure the branding block is set by the minimal install profile so it is there after install.

Moar fix. Leaving the variables in the template so they can be used for other purposes and fixes the installer sitename/wordmark.

We aren't using features to turn that variable on and off as it's now in the block so don't need the conditions.

Also maybe a follow-up but we can remove the code duplication for the branding block in the maintenance/install pages by doing {% include 'block--system-branding-block.html.twig' %}

Whoops I forgot to return the $build in that color #pre_render.