Problem/Motivation

Branding elements have been converted into a block. Let's implement this block in core themes.

Proposed resolution

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

Remaining tasks

  1. Needs manual testing
  2. Needs screenshots
  3. Needs IE test
  4. Needs change record
  5. Needs upgrade path
  6. Needs upgrade path tests

User interface changes

Template markup for branding elements is taken care of by the system branding block template.

API changes

n/a

#1053648-13: Convert site elements (site name, slogan, site logo) into blocks
See the comment: #1053648-13: Convert site elements (site name, slogan, site logo) into blocks

Related to:

Test instructions + Screenshots needed for this issue for Bartik:

  1. Branding block (with Logo, Site Name and Site Slogan enabled) set in the Header - mobile, tablet, desktop layouts.
  2. Branding block (with only Site Name and Site Slogan enabled) set in the Header - mobile, tablet, desktop layouts.
  3. Branding block (with Logo, Site Name and Site Slogan enabled) AND the Search block set in the Header - mobile, tablet, desktop layouts.
  4. Test with the Branding block (with Logo, Site Name and Site Slogan enabled) AND the Search block set in the Header AND another block set in the Header - mobile, tablet, desktop layouts.
  5. RTL tests for all of the above.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because nothing is broken
Issue priority Major because the quest to convert the page template variables to block is half done. It is also a huge site builder win.
Unfrozen changes Unfrozen because it mostly affects the page template.
Disruption Somewhat disruptive as page template variables are being removed, however markup is unfrozen at this point, so should be allowed. There is also an upgrade path to help place the block where possible.
Files: 
CommentFileSizeAuthor
#288 use_branding_block_in-2005546-288.patch44.38 KBhussainweb
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,614 pass(es). View
#284 Screen Shot 2015-09-02 at 01.23.02.png64.85 KBemma.maria
#284 Screen Shot 2015-09-02 at 01.22.57.png84.98 KBemma.maria
#284 Screen Shot 2015-09-02 at 01.22.49.png147.77 KBemma.maria
#283 Screen Shot 2015-09-01 at 11.47.55 AM.png67.39 KBmdrummond
#282 2005546-282-use-branding-block.patch44.38 KBmdrummond
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2005546-282-use-branding-block.patch. Unable to apply patch. See the log in the details link for more information. View
#266 interdiff-230-to-265.txt13.19 KBWim Leers
#230 use_branding_block_in-2005546-229.patch33.54 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch use_branding_block_in-2005546-229.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

lokapujya’s picture

Status: Active » Needs work
FileSize
4.27 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This removes the site elements from the template file and adds the blocks to the installation profile. Changes the header to be floated left.

lokapujya’s picture

This needs to be updated to position the slogan under the site name. Also, we need to figure out what should happen with the html markup that is currently removed by this patch.

hosef’s picture

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.

tim.plunkett’s picture

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

hosef’s picture

FileSize
15.14 KB
16.94 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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.

hosef’s picture

FileSize
417 bytes
16.94 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Ok, fail. New patch.

tim.plunkett’s picture

Issue tags: +Needs screenshots

Can we get some before and after?

oadaeh’s picture

Status: Needs work » Needs review

Updating status.

lokapujya’s picture

One minor issue left over from my original patch: bartik.page_site_slogan status should be 1, not 3

tim.plunkett’s picture

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

tim.plunkett’s picture

Issue summary: View changes

use # link

lokapujya’s picture

Issue summary: View changes

keep a reference to the blocks issue, so the reason for the change is understood.

lokapujya’s picture

Issue summary: View changes

Decouple this issue.

lokapujya’s picture

If we put this in first, then the blocks won't be available and the header will be empty.

tim.plunkett’s picture

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

tim.plunkett’s picture

Issue summary: View changes

reorganizing

lokapujya’s picture

This issue does two things:

  • Remove special case items from themes.
  • Introduce regions as needed to place these elements.
tim.plunkett’s picture

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.

lokapujya’s picture

FileSize
18.69 KB
18.87 KB

So that we have a screenshot to talk about, with Patch from #6 (and the patch from the other issue), I get:
noblocks

With the placed blocks, I get:

With the 3 blocks.png

hosef’s picture

FileSize
6.47 KB
18.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2005546-modify-bartik-for-site-elements-17.patch. Unable to apply patch. See the log in the details link for more information. View
5.13 KB
9.08 KB
23.48 KB
30.82 KB
16.26 KB
21.23 KB
22.92 KB
25.97 KB
13.31 KB
22.84 KB

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:

d7_big.png

d7_small.png

d8_nopatch_big.png

d8_nopatch_small.png

Here is with the patch and with the blocks placed:

d8_patch_blocks_big.png

d8_patch_blocks_sm.png

Here are screenshots with no blocks in either header region:

d8_patch_noblocks_big.png

d8_patch_noblocks_small.png

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

d8_patch_long_elements_big.png

d8_patch_manyblocks_mid.png

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.

hosef’s picture

Issue summary: View changes

Clarify the scope of this issue.

Jeff Burnz’s picture

Category: Feature request » Task
Issue summary: View changes

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.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
14.16 KB
PASSED: [[SimpleTest]]: [MySQL] 64,515 pass(es). View

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.

mdrummond’s picture

FileSize
14.82 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Jeff Burnz’s picture

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

mdrummond’s picture

Great idea, I like it!

Cottser’s picture

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.

mdrummond’s picture

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.

mdrummond’s picture

FileSize
15.77 KB

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.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
23.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,686 pass(es), 37 fail(s), and 7 exception(s). View
19.7 KB

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!

mdrummond’s picture

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.

mdrummond’s picture

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?

mdrummond’s picture

Status: Needs work » Needs review
FileSize
33.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,054 pass(es), 23 fail(s), and 7 exception(s). View
9.78 KB

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.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
35.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,093 pass(es), 15 fail(s), and 7 exception(s). View
4.47 KB

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

joelpittet’s picture

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?

mdrummond’s picture

Status: Needs work » Needs review
FileSize
35.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,082 pass(es), 13 fail(s), and 7 exception(s). View
3.93 KB

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.

joelpittet’s picture

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.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
36.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,097 pass(es), 8 fail(s), and 2 exception(s). View
2.62 KB

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.

mdrummond’s picture

FileSize
30.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,104 pass(es), 8 fail(s), and 2 exception(s). View
6.71 KB

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.

joelpittet’s picture

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

joelpittet’s picture

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.

Jeff Burnz’s picture

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.

joelpittet’s picture

Issue tags: +Needs IE test

@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!

mdrummond’s picture

Status: Needs work » Needs review
FileSize
32.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,044 pass(es), 2 fail(s), and 1 exception(s). View
4.2 KB

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

mdrummond’s picture

FileSize
32.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,173 pass(es), 2 fail(s), and 1 exception(s). View

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

joelpittet’s picture

Status: Needs work » Needs review

testbot engage.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
32.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,232 pass(es), 1 fail(s), and 2 exception(s). View
2.06 KB

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.

Cottser’s picture

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
33.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,245 pass(es). View
2.04 KB

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.

joelpittet’s picture

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

dawehner’s picture

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

mdrummond’s picture

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.

mdrummond’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

mdrummond’s picture

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

Wim Leers’s picture

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

Wim Leers’s picture

Issue tags: +D8 cacheability
rpayanm’s picture

Assigned: Unassigned » rpayanm
rpayanm’s picture

Status: Needs work » Needs review
FileSize
33.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/block/src/Tests/BlockTest.php. View

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

lokapujya’s picture

Where did that code under: // Add a test block.
come from?

rpayanm’s picture

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

rpayanm’s picture

Assigned: rpayanm » Unassigned
lokapujya’s picture

It was part of testBlockRehash() which was removed. So, we can get rid of it. Was moved to testBlockCacheTags() which uses sidebar_first instead of header region, so testBlockCacheTags() won't need any changes.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
32.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 46,729 pass(es), 13,711 fail(s), and 6,781 exception(s). View

Then let me try...

lokapujya’s picture

+++ b/core/includes/theme.inc
@@ -700,14 +700,12 @@ function theme_get_setting($setting_name, $theme = NULL) {
-          $cache[$theme]->set('logo.url', file_create_url($theme_object->getPath() . '/logo.png'));
...
+        $cache[$theme]->set('logo.url', file_create_url(dirname($theme_object->filename) . '/logo.png'));

This shouldn't change.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
32.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,210 pass(es), 20 fail(s), and 1 exception(s). View
702 bytes
lokapujya’s picture

+++ b/core/profiles/standard/config/block.block.bartik_branding.yml
@@ -0,0 +1,22 @@
+  module: system

use provider instead: see https://www.drupal.org/node/1991442

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
32.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,337 pass(es), 23 fail(s), and 1 exception(s). View
501 bytes

wrong patch name, but right patch.

mdrummond’s picture

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’s picture

+++ b/core/modules/system/src/Tests/System/PageTitleTest.php
@@ -73,8 +73,8 @@ function testTitleXSS() {
     // Activate needed appearance settings.
     $edit = array(
-      'toggle_name'           => TRUE,
-      'toggle_slogan'         => TRUE,
+      'toggle_main_menu'      => TRUE,
+      'toggle_secondary_menu' => TRUE,
     );
     $this->drupalPostForm('admin/appearance/settings', $edit, t('Save configuration'));

Should this be removed?

joelpittet’s picture

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
32.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,488 pass(es), 17 fail(s), and 1 exception(s). View

Did #83 (removed that whole block of code). Get the reroll a little closer.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
32.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,475 pass(es), 16 fail(s), and 1 exception(s). View
548 bytes

One more fail down. Maybe this test shouldn't be so delicate. Searching for the nth row seems fragile. But, that's another issue.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Issue tags: +Needs reroll
Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
31.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,462 pass(es), 7 fail(s), and 7 exception(s). View

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

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
32.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,837 pass(es), 35 fail(s), and 25 exception(s). View
1.52 KB
  • Fixed the block not showing up on standard install.
  • Added the block for minimal install profile.
Wim Leers’s picture

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

Manuel Garcia’s picture

Issue tags: +classy

I'm guessing yes.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
31.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,683 pass(es), 28 fail(s), and 1 exception(s). View
667 bytes

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

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
33.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,701 pass(es), 7 fail(s), and 1 exception(s). View
2.31 KB

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.

davidhernandez’s picture

Component: Bartik theme » theme system
Issue tags: +Twig

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.

Manuel Garcia’s picture

FileSize
34.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2005546-100.patch. Unable to apply patch. See the log in the details link for more information. View
635 bytes

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?

Fabianx’s picture

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 )

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
40.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,388 pass(es), 237 fail(s), and 219 exception(s). View
7.93 KB

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

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
42.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,819 pass(es), 1 fail(s), and 1 exception(s). View
2.62 KB

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?

Wim Leers’s picture

Issue tags: +CSS
  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.

mortendk’s picture

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.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
42.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,078 pass(es), 2 fail(s), and 1 exception(s). View
3.55 KB

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!

Wim Leers’s picture

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

Manuel Garcia’s picture

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

mdrummond’s picture

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

Wim Leers’s picture

Issue tags: +TX (Themer Experience)

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.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

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

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Issue tags: +Needs reroll

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

mdrummond’s picture

Priority: Normal » Major

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.

Wim Leers’s picture

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

epari.siva’s picture

Issue tags: -Needs reroll
FileSize
45.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,588 pass(es), 1 fail(s), and 1 exception(s). View

Patch rerolled

JeroenT’s picture

Status: Needs work » Needs review
epari.siva’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
45.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,592 pass(es), 2 fail(s), and 1 exception(s). View

Fixed some bugs.

epari.siva’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
45.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,589 pass(es). View

Sorry. Uploaded wrong patch.

Wim Leers’s picture

Status: Needs review » Needs work

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.

LewisNyman’s picture

  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.

epari.siva’s picture

FileSize
45.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch modify_regions_to-2005546-128.patch. Unable to apply patch. See the log in the details link for more information. View

First a reroll.

Manuel Garcia’s picture

emma.maria’s picture

Status: Needs work » Needs review

Firing testbot to test the patch in #128.

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review
FileSize
45.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,234 pass(es), 1,491 fail(s), and 273 exception(s). View

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
32.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,418 pass(es), 5 fail(s), and 1 exception(s). View
27.82 KB

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.

davidhernandez’s picture

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

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
FileSize
1.19 KB
32.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,432 pass(es). View

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

joelpittet’s picture

FileSize
33.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,425 pass(es). View
6.06 KB

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!

lauriii’s picture

There is still some regression on the visuals

Wim Leers’s picture

Status: Needs review » Needs work

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.
    
    
emma.maria’s picture

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

emma.maria’s picture

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.
 

davidhernandez’s picture

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.

jibran’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
32.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,772 pass(es). View

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.

mark.labrecque’s picture

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.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work

Sounds like it needs work

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
31.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,719 pass(es), 62 fail(s), and 0 exception(s). View

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

joelpittet’s picture

davidhernandez’s picture

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
30.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,770 pass(es), 6 fail(s), and 0 exception(s). View

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
30.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,759 pass(es), 6 fail(s), and 0 exception(s). View
1.83 KB

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' %}

joelpittet’s picture

Status: Needs work » Needs review
FileSize
28.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,014 pass(es). View
28.75 KB

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

Put the variable docs back in the templates. The variables are back mostly so we have some BC left in.

Wim Leers’s picture

No more remarks. Manually tested. Works great.

The only reason I'm not RTBC'ing is because of @lauriii's visual regression testing in #139.

emma.maria’s picture

Assigned: Unassigned » emma.maria
Status: Needs review » Needs work

I will add the CSS fixes for Bartik, this patch visually is far from ready :)

zetagraph’s picture

Assigned: emma.maria » zetagraph

Taking this from Emma, she gave me some pointers, will look into visual regressions

zetagraph’s picture

FileSize
29.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,352 pass(es). View
2.55 KB

Cleaned up the header styles, removed redundant margins in header branding. Floated branding to the left and the search to the right (opposite for RTL). Also added region--header.htm.twig with a clearfix class.

zetagraph’s picture

Assigned: zetagraph » Unassigned
Status: Needs work » Needs review
zetagraph’s picture

FileSize
30.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,361 pass(es). View
928 bytes

Adding the missing region--header.html.twig

lauriii’s picture

Status: Needs review » Needs work
+++ b/core/themes/bartik/templates/region--header.html.twig
@@ -0,0 +1,16 @@
\ No newline at end of file

New line should be added to end of this file :)

joelpittet’s picture

FileSize
616 bytes
30.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,502 pass(es). View
616 bytes

Here's that fix. The CSS is really the only thing that needs review FWIU.

Wim Leers’s picture

Status: Needs work » Needs review
joelpittet’s picture

Title: Modify regions to support Convert site elements (site name, slogan, site logo) into blocks » Use branding block in place of page template branding variables (site name, slogan, site logo)
Issue summary: View changes

Better title?

Fabianx’s picture

Much better!

lauriii’s picture

Status: Needs review » Needs work

There is still some visual regression especially on anonymous users. I have tried to point the reasoning for that:

  1. +++ b/core/themes/bartik/css/components/header.css
    @@ -6,99 +6,65 @@
    +  .region-header .branding {
    ...
    +  }
    

    So the branding block has smaller font size because there is .region-header .block { font-size: 0.857em; } so we need to override that for .region-header .branding.

  2. +++ b/core/themes/bartik/css/components/header.css
    @@ -6,99 +6,65 @@
     .site-logo {
       float: left; /* LTR */
    -  padding: 4px 4px 4px 9px; /* LTR */
    +  padding: 4px 20px 10px 0; /* LTR */
     }
    -[dir="rtl"] #logo,
     [dir="rtl"] .site-logo {
       padding: 4px 9px 4px 4px;
     }
    

    RTL is missing the change

  3. +++ b/core/themes/bartik/css/components/header.css
    @@ -6,99 +6,65 @@
    -    padding: 9px 4px 4px 9px; /* LTR */
    +    padding: 0 19px 0 0; /* LTR */
       }
    -  [dir="rtl"] #logo,
       [dir="rtl"] .site-logo {
         padding: 9px 9px 4px 4px;
       }
    

    This is missing changes for the RTL. This also visually breaks the branding block on anonymous users because there is no padding on the top.

  4. +++ b/core/themes/bartik/css/components/header.css
    @@ -252,12 +215,10 @@ h1.site-name {
    \ No newline at end of file
    

    One new line from the end of file missing

lauriii’s picture

Assigned: Unassigned » lauriii

Will fix this

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
30.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch use_branding_block_in-2005546-171.patch. Unable to apply patch. See the log in the details link for more information. View
1.36 KB
47.88 KB
38.29 KB
48.65 KB
39.03 KB
39.81 KB
30.2 KB
39.44 KB
29.66 KB

I still didn't test bartik with mobile.

Before:

After:

Before:

After:

Before:

After:

Before:

After:

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
30.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,545 pass(es). View

Interdiff for the previous patch and reroll

bill richardson’s picture

Status: Needs review » Reviewed & tested by the community

Tested in English and Arabic -- branding block displays logo,etc without any visible differences - ready to commit.

davidhernandez’s picture

Issue tags: +Needs change record

There is some backwards compatibility in this, but given some of the variable changes and the very significant changes to the page templates, there should be a change record.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
60.62 KB

After standard install it looks like this...

... looks like something went wrong with the reroll - or it's alexpott error :(

Needs works for #175 anyway - good call @davidhernandez

alexpott’s picture

Interestingly I only see #176 in Chrome (on a Mac) and not Firefox and only for anonymous users. No amount of cache clearing on anything fixes it.

joelpittet’s picture

Too many floats! The branding is floating left and the contents are also floating left.

bill richardson’s picture

Also having issues in Chrome as anonymous user -- I think solution is to give the branding block a width in css, rather than there being any extra floating left properties.

emma.maria’s picture

Assigned: Unassigned » emma.maria
Issue summary: View changes

I have noticed a lot of problems with the CSS which are causing all of these visual regressions. Mobile styles are missing from within Branding, plus no one tested adding a second block to the header which is super important as we are adding multiple behaviours to blocks in the header. I am currently working on a patch to fix everything and will post a patch tomorrow (Sunday).

@alexpott I have noticed what you have posted in #176 too, it's definitely not an @alexpott error.

@bill richardson we can't put a width on the branding block, people need to add any length of title / logo / slogan which will need to span as wide as needed.

@joelpittet I'm working on fixing the many floats.

Note for review: I have added test instructions for this issue for Bartik, due to the added complexity to the block layout behaviour in the header

emma.maria’s picture

Assigned: emma.maria » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs screenshots, +Needs manual testing
FileSize
32.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,243 pass(es). View
56.2 KB
53.66 KB
41.03 KB
64.8 KB
59.72 KB
50.16 KB
6.02 KB

I have overhauled the markup and CSS for Bartik.

Main points to take note when reviewing:

  1. As site-branding is now a proper component I created it's own CSS file in Bartik.
  2. I adjusted the markup in the Bartik site branding Twig template to follow BEM.
  3. I had to add styles to not take into account the branding block when styling blocks added to the Header region. The design has vertical spacing for any block which branding needs to ignore.
  4. I added styles to account for the branding block being in the region on it's own and also with other blocks. The spacing between the branding and search and then the bottom of the header are smaller amounts than the space between just the branding and bottom of the Header.
  5. The original Bartik design for small viewports had the search block floated to the right. I have added this as a search component specific style instead of every block in the Header. Any other block added to the region will float left and be full width.
  6. I added the padding: 0 15px; style to the Header region container and took a ton of padding left and right off the elements within it. The region should control the horizontal alignment/widths and should match the rest of the regions on the page. This means that everything will have the correct spacing from the sides at all widths no matter what is added. This has also caused a visual change in design where the logo does not poke out the left side (SO happy this is gone!). Please keep this it's such an improvement!
  7. There may be a slight change in the vertical spacing between the branding block and any other block added to the Header. This is expected as all of the branding is now wrapped in one container now so nothing can sneak into any spaces higher than the bottom of the whole of the site branding container. (The search used to sneak under the branding text and next to the logo but it physically can't now which is correct).

If you have any questions about the CSS choices I've made, please let me know. I've spent many hours today working things out :)
Also there is an issue open to properly clean up the CSS for the header in Bartik here #2466983: Clean up the "header" component in Bartik, so code can also be fixed there to speed this issue up.

Screenshots

Branding block on it's own


 

 

 

Branding block with Search block and Powered by block


 

 

 
I also checked the RTL styles before creating the patch and they seem fine.

Patch and interdiff attached.

Wim Leers’s picture

Wow, thanks for the amazing work, Emma!

I don't think I'm qualified to review this anymore, so deferring to folks with stronger theming/CSS skills.

bill richardson’s picture

If site slogan not added - name is not properly aligned.
image supplied to show issue

dcrocks’s picture

Actually, vertical alignment of site title with logo changes with screen width in #181 as well, both alone and with other blocks.

bill richardson’s picture

Status: Needs review » Needs work
emma.maria’s picture

@dcrocks The current design has the site title sitting around the middle for desktop and then top aligned for tablet and mobile.

@bill richardson Ack I haven't accounted for the slogan not being present when I did the middle alignment site title styling so it sits at the bottom of the container currently. This needs work.

Bartik really needs screenshots comparing before and after and taking into considerations how the new markup affects alignment per my notes in #141.

bill richardson’s picture

Status: Needs work » Reviewed & tested by the community

Would be happy to rtbc this - leave any small cosmetic changes to normal issue 2466983 ( clean up the "header" component in Bartik)

emma.maria’s picture

Status: Reviewed & tested by the community » Needs work

No it's not ready.

I don't want visual regressions committed for Bartik. Plus Bartik needs screenshots of all the scenarios with everything looking great to be signed off. This was a huge markup and CSS change for Bartik so it's being committed without bugs.

Plus @davidhernandez has pointed out that this issue needs a change record.

mdrummond’s picture

Two things:

1) Bartik uses .site-branding__text while the maintenance page uses .site-branding-text. Is it possible to have maintenance page use site-branding__text as well?

2) This is an issue that is probably larger than this patch, but there's a mix of px and em in a number of places. It would be great if this could be more consistent. If I had my druthers, I'd use em in media queries and rem everywhere else. Although with a postprocessor with a pxtoem conversion, having everything as px in css is handy. That's not an option here, though. Anyhow, like I said, my guess is that this is a mix throughout the css codebase, and we may not have enough time normalize this.

Markup and CSS looks good to me at first glance. I might need to look through more carefully to better understand how things have changed as styles have been moved into the component css file (which is great by the way).

Nice work!

LewisNyman’s picture

@mdrummond

1) This decision probably deserves a wider discussion on what 'site-branding-text' actually means as a visual component. It feels like a follow up to me if that's ok with you.

2) #2298015: [policy] Document when we should use each CSS unit

HOG’s picture

Some changes for header blocks theming.

bill richardson’s picture

Is patch 191 to be applied after patch 181 ?

HOG’s picture

181 patch was in branch as seen, because i could not apply 181 patch.

bill richardson’s picture

Hog --- patch 181 has not been commited yet --- so not sure which branch you are working from.
There is another issue 2466983 to fix header in Bartik , maybe that was the issue you wanted to post to.

HOG’s picture

FileSize
33.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,770 pass(es). View
33.68 KB

New patch with 181.

andypost’s picture

Status: Needs work » Needs review

looks re-roll, cleaned attached files

bill richardson’s picture

Status: Needs review » Needs work

There is still a problem, when no slogan used , the title is not in the correct position.

emma.maria’s picture

Assigned: Unassigned » emma.maria
emma.maria’s picture

The patch in #191 adds back CSS code removed in previous patches, plus the selectors also do not exist in the markup anymore.

+++ b/core/themes/bartik/css/components/header.css
@@ -6,112 +6,92 @@
-@media all and (min-width: 461px) and (max-width: 900px) {
-  .region-header {
-    margin: .5em 5px .75em;
-  }
+#logo,
+#name-and-slogan,
+.region-header {
+  -moz-box-sizing: border-box;
+  -webkit-box-sizing: border-box;
+  box-sizing: border-box;
+  display: inline-block;
+  max-width: 100%;
+  vertical-align: middle;
 }
-@media all and (min-width: 901px) {
-  .region-header {
-    margin: 1em 5px 1.5em;
-  }
+#name-and-slogan {
+  color: #fff;
+  width: 43%;
 }
-#logo,
-.site-logo {
-  float: left; /* LTR */
-  padding: 4px 4px 4px 9px; /* LTR */
+#name-and-slogan a {
+  color: #fff;
 }

The patch in #195 also introduces visual regressions for eg.

I will fix these things up plus fix the issues addressed in #189 and #197.

HOG’s picture

@bill richardson, emma.maria

1. Title must be in center of the logo?
2. Logo, site name, site slogan - always in the left of header?

HOG’s picture

FileSize
81.09 KB

So, after applying 181 path i see this structure:

html strrucrure

It's right?

bill richardson’s picture

Hog -- did you clear cache and enable branding block in header region.
The only changes required after patch 181 are some small css changes to correct placement of title with / or without slogan.

emma.maria’s picture

Assigned: emma.maria » Unassigned
Status: Needs work » Needs review
FileSize
1.84 KB
32.74 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch use_branding_block_in-2005546-203.patch. Unable to apply patch. See the log in the details link for more information. View

I have fixed visual issues that were missed or introduced by myself in #181. These were very small cosmetic fixes and did not require an overhaul of the code.

emma.maria’s picture

@mdrummond : I checked the Maintenance page. The maintenance page markup needs a huge overhaul in general with it's markup and CSS and I feel like that can be worked on in a follow up issue. The maintenance page also does not load the branding block so it's sort of out of scope to this issue.

lauriii’s picture

Status: Needs work » Needs review
FileSize
33.16 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,850 pass(es). View

Rerolled the patch

bill richardson’s picture

Patch 206 displays logo,etc as a block and title / slogan retain the correct positions.

LewisNyman’s picture

Status: Needs review » Needs work

The only thing I noticed when manually testing this patch was that there is a blank space about the header. You can only see it when anonymous as the toolbar covers it up. This is because the header region has a top margin. Using padding instead should fix this.

HOG’s picture

FileSize
79.01 KB
44.14 KB
28.7 KB
66.67 KB
38.77 KB
24.6 KB

Now i see regression on the desktop when slogan missing.
v1-desktop
v1-tablet
v1-mobile
v2-desktop
v2-tablet
v2-mobile

emma.maria’s picture

Changing individually printed fields (logo, title, slogan) to be contained within one block will definitely create some visual differences with layouts, especially when printing more than one component in the header. There can be visual bugs in the patches but not everything can be classified as a visual regression, we are overhauling the header markup, behaviour and CSS plus adding a new branding block.

I will add screenshots comparing before and after plus justifying the visual differences we have between them. The header looks so so so(!) much better now and I am very happy with the progress.

The patch needs to be fixed one more time though.

@lauriii has accidentally included changes to maintainers.txt in the patch. This is the only file that needs to be removed.

emma.maria’s picture

Changing individually printed fields (logo, title, slogan) to be contained within one block will definitely create some visual differences with layouts, especially when printing more than one component in the header. There can be visual bugs in the patches but not everything can be classified as a visual regression, we are overhauling the header markup, behaviour and CSS plus adding a new branding block.

I will add screenshots comparing before and after plus justifying the visual differences we have between them. The header looks so so so(!) much better now and I am very happy with the progress.

The patch needs to be fixed one more time though.

@lauriii has accidentally included changes to maintainers.txt in the patch. This is the only file that needs to be removed.

jibran’s picture

+++ b/core/MAINTAINERS.txt
-- Jen Lampton 'jenlampton' https://www.drupal.org/u/jenlampton
+- Lauri Eskola 'lauriii' https://www.drupal.org/u/lauriii

@lauriii nice way to make it official ;-)

wmortada’s picture

Status: Needs work » Needs review
FileSize
32.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,954 pass(es). View
714 bytes

I have removed the offending file from the patch and attach an updated patch and interdiff.

lauriii’s picture

We still need upgrade path for this :(

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

+++ b/core/modules/color/color.module
@@ -120,6 +121,27 @@ function color_preprocess_page(&$variables) {
+  return $build;

Extra line before return is nice :)

wmortada’s picture

FileSize
496 bytes
32.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,952 pass(es). View

I've added the extra line.

rteijeiro’s picture

Issue summary: View changes
FileSize
49.25 KB
10.14 KB
32.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,666 pass(es). View
1.14 KB

Fixed a few nits. It looks nice!

Tested adding and removing branding elements. No issues found.

Check the screenshots:

LOGO BLOCK

LOGO BLOCK CONFIG

rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
9.94 KB

Just found a small issue when not logged in. It appears a small black gap on the top. Not sure if it has something to do with what @lewisnyman reported in #209

Back to needs work, sadly :(

mdrummond’s picture

FileSize
32.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,673 pass(es). View
429 bytes

This should fix the black space at the top of anonymous pages. It sounds like that's the last blocker to RTBC for this. There are some other visual changes, but Emma Maria said she would document the reasons for those. I think that would be good to get in here.

mdrummond’s picture

Status: Needs work » Needs review

Bring it, Testbot.

lauriii’s picture

+++ b/core/includes/theme.inc
@@ -1325,8 +1322,8 @@ function template_preprocess_page(&$variables) {
-  $variables['site_name']         = (theme_get_setting('features.name') ? SafeMarkup::checkPlain($site_config->get('name')) : '');
-  $variables['site_slogan']['#markup'] = (theme_get_setting('features.slogan') ? $site_config->get('slogan') : '');
+  $variables['site_name']         = $site_config->get('name');
+  $variables['site_slogan']       = $site_config->get('slogan');

We want to get rid of the variables on page.html.twig so we should a test whether we're creating the page for installer/maintenance page and only then add the variables. After doing that we can get rid of $site_config = \Drupal::config('system.site'); on normal page load.

mdrummond’s picture

FileSize
32.72 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,682 pass(es), 0 fail(s), and 31 exception(s). View
935 bytes

This removes the page variables except on maintenance and install pages. It is entirely possible that makes something go kaboom.

And that is why we love Testbot.

mdrummond’s picture

FileSize
716 bytes
33.27 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,677 pass(es), 0 fail(s), and 31 exception(s). View

Also removing unneeded $site_config. Thanks Laurii!

lauriii’s picture

+++ b/core/includes/theme.inc
@@ -1310,7 +1307,11 @@ function template_preprocess_html(&$variables) {
+  if (defined('MAINTENANCE_MODE')) {
+    $site_config = \Drupal::config('system.site');
+  }

@@ -1324,9 +1325,13 @@ function template_preprocess_page(&$variables) {
+  if (defined('MAINTENANCE_MODE')) {

We can move the $site_config to the second if. No need for two separated ifs

lauriii’s picture

Assigned: Unassigned » lauriii
tstoeckler’s picture

+++ b/core/includes/theme.inc
@@ -1310,7 +1307,11 @@ function template_preprocess_html(&$variables) {
-  $site_config = \Drupal::config('system.site');
+
+  // $site_config only needed on maintenance and install paths.
+  if (defined('MAINTENANCE_MODE')) {
+    $site_config = \Drupal::config('system.site');
+  }

@@ -1324,9 +1325,13 @@ function template_preprocess_page(&$variables) {
-  $variables['logo']              = theme_get_setting('logo.url');
-  $variables['site_name']         = (theme_get_setting('features.name') ? SafeMarkup::checkPlain($site_config->get('name')) : '');
-  $variables['site_slogan']['#markup'] = (theme_get_setting('features.slogan') ? $site_config->get('slogan') : '');
+
+  // Only generate branding variables on maintenance and install paths.
+  if (defined('MAINTENANCE_MODE')) {
+    $variables['logo'] = theme_get_setting('logo.url');
+    $variables['site_name'] = $site_config->get('name');
+    $variables['site_slogan'] = $site_config->get('slogan');
+  }

We have a template_preprocess_maintenance_page() (which calls out to template_preprocess_page()) so wouldn't it make more sense to simply move these there?

Wim Leers’s picture

#228 sounds like a great idea.

lauriii’s picture

Status: Needs work » Needs review
FileSize
33.54 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch use_branding_block_in-2005546-229.patch. Unable to apply patch. See the log in the details link for more information. View
2.19 KB

Fixed tests and addressed open feedback. I was thinking of the upgrade path but its going to be quite useless for the custom themes because of all the unknowns.

joelpittet’s picture

Issue summary: View changes
FileSize
30.46 KB
112.52 KB

This is looking really good from the code huge +1. And the look of it in the browser it's good too. (There is a slight but nice visual change to the position, but it's better aligned with other elements so +1 to that change)

joelpittet’s picture

Issue summary: View changes
FileSize
59.48 KB

Better deskop screencast

joelpittet’s picture

Issue summary: View changes
FileSize
50.27 KB
57.79 KB

Slogan is awesome and emoji's work:D

Slogan

Edit: Search was user-error.

emma.maria’s picture

joelpittet’s picture

Issue summary: View changes
Issue tags: -Needs screenshots, -Needs manual testing
FileSize
84.75 KB

There was no issue with the search at all. There is a bug in the Blocks UI that makes impossible to drag and drop between regions sometimes. I'll go look for an issue for that.

Anyways the search is fine:)

joelpittet’s picture

Issue summary: View changes
Issue tags: -Needs IE test
FileSize
909.06 KB
666.26 KB

Here is IE9 and IE11 other than emoji fail it's looking fine.

IE 9

IE 11

joelpittet’s picture

Issue summary: View changes
Issue tags: -Needs change record

Here's a draft CR. Feel free to update as you see fit. https://www.drupal.org/node/2544012

Wim Leers’s picture

#233++

CR looks great.

I think this means this really needs just an upgrade path now, and then it can be RTBC?

dcrocks’s picture

Probably shouldn't mention region names in the CR as they might change. If a theme doesn't have the correctly named region the block is placed in the default region or disabled.

andypost’s picture

If a theme doesn't have the correctly named region the block is placed in the default region or disabled.

the block should be saved as disabled into properly named region

bill richardson’s picture

Status: Needs review » Needs work

see #238.#240 -- setting to needs work.

lauriii’s picture

Assigned: lauriii » Unassigned
dcrocks’s picture

Issue tags: +Needs reroll

re: #240, Blocks are listed 'disabled' if they are not assigned to a region. Are you saying to create an issue to give blocks an enabled/disabled status as part of their configuration?

re: #238, Given the number of user files that might refer to the replaced variables, how do you safely provide an upgrade path?

And the patch no longer applies and needs a re-roll.

Shouldn't this go in before beta?

tim.plunkett’s picture

#240, that's #2513534: Remove the 'disabled' region from Block UI, please stay in scope here.

madhavvyas’s picture

Status: Needs work » Needs review
FileSize
33.43 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,504 pass(es), 258 fail(s), and 62 exception(s). View

Patch re rolled for comment 230

vermauv’s picture

Issue tags: -Needs reroll
mdrummond’s picture

Status: Needs work » Needs review
FileSize
33.52 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,878 pass(es), 2 fail(s), and 0 exception(s). View

Errors in the reroll. Here's a fresh one. Thanks for the effort, madhavvyas.

mdrummond’s picture

If we're going to have an upgrade path, it seems as if it should do the following:

For each enabled theme:

1) Is there a branding block already placed in a region? If so, do nothing.

2) If a branding block has not been placed, is there a region named 'header'? If not, do nothing.

3) If there is a header region, prepare a branding block. Check the theme appearance settings for the toggles for logo, site name and slogan. Apply those settings to a new branding block and place it in that region.

----

Honesty, I'm not sure how many sites will fit the criteria where an upgrade path would prove useful. Promoting the change notice so those with d8 sites can make sure a branding block gets placed may be a better option. This is a change people will notice, and they will want to take action. If we can smooth out that process for the few where that would be helpful, okay, but it will not be possible to create an upgrade path that will result in zero disruption.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
34.97 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,846 pass(es), 0 fail(s), and 2 exception(s). View
1.45 KB

This should fix the config test errors.

mdrummond’s picture

mdrummond’s picture

FileSize
35.54 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,892 pass(es). View
1.18 KB

Fixing the exceptions by requiring the block module in the broken tests.

mdrummond’s picture

Status: Needs work » Needs review
bill richardson’s picture

Happy with patch - just needs an update path.

mdrummond’s picture

Issue summary: View changes
Issue tags: -Needs upgrade path

Here's a first shot at the upgrade path. We'll still need tests for this upgrade path.

I adapted the upgrade path in #507488: Convert page elements (local tasks, actions) into blocks since it is very similar. Since both need a helper function for creating blocks, I didn't put the update number into the function title. Creating a separate helper function for both patches seems redundant.

The upgrade path I've added is simpler than what I described in #251, rightly or wrongly. Feedback welcome.

mdrummond’s picture

FileSize
3.5 KB
39.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,961 pass(es). View

Here are the files. Blergh.

mdrummond’s picture

Issue summary: View changes
Issue tags: -Needs upgrade path tests
FileSize
44.39 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,245 pass(es), 0 fail(s), and 2 exception(s). View
5.35 KB

Here is the upgrade path tests, again based off of the work in #507488: Convert page elements (local tasks, actions) into blocks.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
44.39 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,252 pass(es), 1 fail(s), and 0 exception(s). View
734 bytes

Fixing test exceptions.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
44.4 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2005546-265-use-branding-block.patch. Unable to apply patch. See the log in the details link for more information. View
850 bytes

Slogan is empty by default, so the test logic was backwards.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
13.19 KB

This was ready in #238 (the patch in #230 was ready, and Joël Pittet did extensive visual regression tests in #231–#237). All that was left, was an upgrade path. I reviewed the interdiffs here, and also am posting a patch-230-to-patch-265 interdiff directly, to make it clear what happened in those last 30 comments. As you can see, no CSS or template changes since #230.

I also manually tested the upgrade path, and experienced no problems.

Thanks for pushing this to the finish line, @mdrummond!

  1. +++ b/core/modules/system/system.install
    @@ -1247,3 +1247,97 @@ function system_update_8003() {
    +        $values = [
    +            'id' => 'bartik_branding',
    +            'region' => 'header',
    +          ] + $site_branding_default_settings;
    ...
    +        $values = [
    +            'id' => 'stark_branding',
    +            'region' => 'header',
    +          ] + $site_branding_default_settings;
    ...
    +        $values = [
    +            'id' => sprintf('%s_branding', $theme_name),
    +            'region' => 'content',
    +            'weight' => '-50',
    +          ] + $site_branding_default_settings;
    

    The indentation of the quoted lines under $values = [ is a bit off: indented two spaces too many. Can be fixed on commit.

  2. +++ b/core/modules/system/system.install
    @@ -1247,3 +1247,97 @@ function system_update_8003() {
    +        break;
    +      default:
    

    Nit: missing \n between these two lines. Can be fixed on commit.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
44.82 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2005546-268-use-branding-block.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll due to new system update number. Should be fine to move back to RTBC once the patch is green.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
lauriii’s picture

This is colliding with #507488: Convert page elements (local tasks, actions) into blocks which is also RTBC. If possible commit the local tasks one first because it has almost 500 comments and then reroll this one.

mdrummond’s picture

Doing a straight-up reroll, but this is going to require some refactoring, because the big change was the addition of the following:

/**
 * Helper function for handling the site name and slogan.
 */
function _bartik_process_page(&$variables) {
  $site_config = \Drupal::config('system.site');
  // Always print the site name and slogan, but if they are toggled off, we'll
  // just hide them visually.
  $variables['hide_site_name']   = theme_get_setting('features.name') ? FALSE : TRUE;
  $variables['hide_site_slogan'] = theme_get_setting('features.slogan') ? FALSE : TRUE;
  if ($variables['hide_site_name']) {
    // If toggle_name is FALSE, the site_name will be empty, so we rebuild it.
    $variables['site_name'] = $site_config->get('name');
  }
  if ($variables['hide_site_slogan']) {
    // If toggle_site_slogan is FALSE, the site_slogan will be empty, so we
    // rebuild it.
    $variables['site_slogan']['#markup'] = $site_config->get('slogan');
  }
}

I thought the process layer was removed, but anyhow, this needs to affect the variables in the block template now, rather than the page template, since those variables are no longer in that template.

I think I remember seeing the issue that led to this change. I'll track it down to better understand this.

mdrummond’s picture

Okay, there was just one line changed by #2557871: Remove all usages SafeMarkup::checkPlain() from template preprocess functions and attributes that was removing a SafeMarkup::checkPlain() around getting the site name from site config. I'll double-check the patch to see where that code got moved. We should be able to remove the _process chunk.

mdrummond’s picture

FileSize
43.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Forgot to post the reroll patch.

mdrummond’s picture

Status: Needs work » Needs review

Okay, we had removed that whole chunk before, and we weren't using that checkPlain call anywhere else, so this should get us back in shape.

If this is green, should be fine to move back to RTBC. Ideally we'd want https://www.drupal.org/node/507488 to get in first, then reroll this hopefully one last time to fix the update number.

mdrummond’s picture

FileSize
44.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.18 KB

And here's the correct patch.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
45.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,436 pass(es). View
1.09 KB

Fixing the update path test.

Wim Leers’s picture

Status: Needs review » Needs work
FileSize
1.38 KB

#268 -> #279 patch-based interdiff attached. No actual changes besides catering for UpdatePathTestBase changes and SafeMarkup changes.

… but … after a fresh install, the branding block now sits on the right-hand side instead of the left :( So, seems like some of the CSS in HEAD has changed. So unfortunately not back to RTBC.

emma.maria’s picture

Issue tags: +Needs manual testing

Hmmm I will take a look at whats going on visually.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
44.38 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2005546-282-use-branding-block.patch. Unable to apply patch. See the log in the details link for more information. View

Here's a reroll since https://www.drupal.org/node/507488 got in.

mdrummond’s picture

Just tested this on simplytest.me. Branding block is right where it is supposed to be.

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
147.77 KB
84.98 KB
64.85 KB

I visually tested with simplytest.me all looks fine :)
 

 

 

 
I am going to do the honours and set this to RTBC :D

Bojhan’s picture

Whoo, lets do this! :) Thanks.

Couldnt reproduce the issue from Wim

Wim Leers’s picture

I'm pretty sure now it was a browser cache problem: Chrome frequently seems to need a *double* hard refresh (Cmd/Ctrl+R) to actually refresh CSS/JS. It indeed works perfectly :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 282: 2005546-282-use-branding-block.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
44.38 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,614 pass(es). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 288: use_branding_block_in-2005546-288.patch, failed testing.

hussainweb’s picture

Can someone verify if GET http://ec2-54-191-110-230.us-west-2.compute.amazonaws.com/checkout/update.php/results returned 0 (0 bytes). is a valid failure? I don't think this has anything to do with this issue and could be random. Also, I see the test on DrupalCI has passed.

Status: Needs work » Needs review
mdrummond’s picture

Looks like a random failure. Triggered a re-test. Thanks for the reroll!

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

Yay, patch is green! Moving back to RTBC.

catch’s picture

Assigned: Unassigned » webchick
+++ b/core/modules/system/system.install
@@ -1462,7 +1463,78 @@ function system_update_8005() {
+    return t('Block module is not enabled so site branding elements, which have been converted to a block, are not visible anymore.');

Isn't it up to page manager to also have an update? Do any sites just turn off block module and leave it like that? Won't custom themes need updating whether block is updated or not?

Patch looks great otherwise. Assigning to webchick in case she wants to take this one.

  • webchick committed 2993ad5 on 8.0.x
    Issue #2005546 by mdrummond, joelpittet, Manuel Garcia, lauriii,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

#294 is a valid question, but we could massage this message more in a normal follow-up, methinks.

I'm not exactly sure why this is assigned to me, but if all the folks who've commented on this so far don't have any complaints, not sure I would either. :) This also falls under the class of one of those kinds of patches that a) is completing pre-existing work, and b) would be vastly better to get in prior to RC, so I'm generally +1.

I clicked around with this a bit manually. The upgrade path works, you see the contextual links hover as expected (unfortunately you can't configure any of the relevant settings such as site logo and name; instead the block config directs you to elsewhere, but that's not a new problem). Too bad we don't have quick edit there, but one step at a time. ;) Also installed minimal and the branding block is there in Stark as well. Yay!

The one issue I did find is that when switching from Bartik to Classy as your front-end theme, the site branding block goes away. I talked to mdrummond about this and he said that's a Classy-specific thing, since Classy doesn't enable any blocks. There's an issue to discuss this further at #2562987: Enable blocks in testing profile and/or Classy. Depending on your POV, this is either a regression from HEAD, in that people using Classy now have to do some manual futzing to get something that used to show up automatically to show up. Or, it's a good thing, because it makes the behaviour of blocks consistent throughout.

I kind of lean more towards the former (it's weird that Stark of all things gets this right but Classy (which is supposed to be the "do more for you" theme) doesn't, but since we have a dedicated follow-up to discuss it, I think that's fine to punt for now.

Anyway, can't find anything else to complain about here, sooooo...!

Committed and pushed to 8.0.x. YEAH! Great work on this one, folks!!

dcrocks’s picture

"SiteBrandingConvertedIntoBlockUpdateTest.php" is a cut and paste and then modify copy of "LocalActionsAndTasksConvertedIntoBlocksUpdateTest.php" and unfortunately still kept the same documentation comments of the latter, eg.:

/**
* Tests the upgrade path for local actions/tasks being converted into blocks.
*
* @see https://www.drupal.org/node/507488

That file is giving errors on a patch I am working on for #2513526: Rename the menu regions. I can fix those comments there while I try to fix my problem there unless that is a no-no.

davidhernandez’s picture

Sure, unless you think it is too out of scope for the other issue. In that case, the comment change can be done in a separate issue.

longwave’s picture

I opened #2566579: Use sentence case for branding block label as "Site Branding" looks inconsistent to me when compared to the other block labels.

dcrocks’s picture

re: #298 Ok, this s trivial so I'll make changes in my patch.

Wim Leers’s picture

+++ b/core/modules/system/system.install
@@ -1462,7 +1463,78 @@ function system_update_8005() {
+    'settings.cache.max_age' => 0,

This line sadly means that any D8 site that already existed will get a branding block (yay!) that is marked to not ever be cached (boo!).

This line simply does not belong here. So… it looks like we need a new update function that takes the site branding block and removes this setting.

dcrocks’s picture

From my comment in #297, I ended up not having to touch that code so I created a followup. Are there other thing that might get stuck in there?

webchick’s picture

Let's get a follow-up for that upgrade path issue but AFAIK we can just delete the line as long as it happens in the next few days. We only support upgrade path from version X to version Y, not HEAD to HEAD, and there hasn't been a new core release since this was committed.

dcrocks’s picture

Status: Fixed » Closed (fixed)

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