Problem/motivation

Drupal 7 has most global site and page elements hardwired into templates, and gives no control to users to move them around. All the page elements (site name, slogan, site logo) are hardwired to core and contrib themes alike. We should stop special-casing these, so they can be moved around and selectively hidden or replaced as needed.

Proposed solution

  • Introduce them as specific blocks that can be moved around.

To be done here #2005546: Modify regions to support Convert site elements (site name, slogan, site logo) into blocks:

  • Remove special case items from themes.
  • Introduce regions as needed to place these elements.

Related issues

The code should depend on blocks as plugins (#1535868: Convert all blocks into plugins), however, we should not mark this postponed so we can do work on this in parallel and be ready for commits once that lands.

Also:

Files: 
CommentFileSizeAuthor
#203 interdiff.txt1.65 KBCottser
#203 1053648-203.patch24.21 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,758 pass(es).
[ View ]
#202 interdiff-1053648-198-202.txt2.97 KBmdrummond
#202 site-branding-block-1053648-202.patch24.2 KBmdrummond
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,760 pass(es).
[ View ]
#201 1053648-201-bartik.png28.28 KBCottser
#187 sitebrand2.jpg66.43 KBdcrocks
#187 sitebrand.jpg63.36 KBdcrocks
#141 Screen Shot 2014-02-22 at 10.48.59 PM.png67.87 KBwebchick
#141 Screen Shot 2014-02-22 at 10.53.54 PM.png49.87 KBwebchick
#135 site-branding-block-1053648-135.patch11.57 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 64,605 pass(es).
[ View ]
#93 siteblock.jpg100.09 KBdcrocks
#66 asample.jpg62.87 KBdcrocks
#58 sysbtand.jpg61.29 KBdcrocks
#24 bart.png87.2 KBdcrocks
#13 TopRegions.png28.25 KBGábor Hojtsy

Comments

droplet’s picture

??

content is block in D7

subcomandante’s picture

exactly, maybe I wrote bad.
I think it could be nice to have others "in core" blocks as well like the ones I mention above.

tstoeckler’s picture

Version:7.x-dev» 8.x-dev

mission and footer are already a block.
the rest is D8 material.

Scott J’s picture

Michele,
Have you found Blockify module?

stevector’s picture

Status:Active» Closed (duplicate)

Major changes along these lines are in progress at http://drupal.org/sandbox/eclipsegc/1441840

David_Rothstein’s picture

Status:Closed (duplicate)» Active

This is a relatively small, focused issue, so it should remain open.

But see also #507488: Convert page elements (title, local tasks, actions) into blocks which is closely related (and has a patch). Although they focus on "blockify-ing" different things, it may make sense to merge them.

David_Rothstein’s picture

Title:Everything as a block» Make the site name, logo, and slogan into blocks

Although I forgot to change the issue title to reflect its actual focus :)

Gábor Hojtsy’s picture

Issue tags:+Blocks-Layouts, +B&L

Tagging it up.

Gábor Hojtsy’s picture

Title:Make the site name, logo, and slogan into blocks» Convert site elements (site name, slogan, site logo) into blocks
Component:block.module» theme system
Status:Active» Needs review
StatusFileSize
new3.08 KB
FAILED: [[SimpleTest]]: [MySQL] 55,761 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

All right, so given #1535868: Convert all blocks into plugins, it probably does not make much sense at this point to convert these to blocks as blocks exist pre-plugins. However, @EclipseGC actually did lots of the conversion work that is involved with these blocks in interim patches at #1787846: Themes should declare their layouts. It was not actually related to introducing layouts as a concept, and I want to make sure we have the patch around, so here it is. All the code included is from @EclipseGC.

Blocks included:
- site name
- site logo
- site slogan

Technically this issue does not make much sense until #1535868: Convert all blocks into plugins however, I think it would make sense to parallelise work. This needs to be kept up to date with the API there.

ParisLiakos’s picture

#9: site-elements-as-blocks-7.patch queued for re-testing.

Gábor Hojtsy’s picture

Have been looking at #1535868: Convert all blocks into plugins for ways to move this forward. I believe:

1. We'd need to introduce a new region to place these blocks (current header regions have a different setup).
2. We'd need to add the new block to the profiles in core as CMI configs. See below for help snippet from the patch.
3. We'd need to remove the template variables related to these blocks.

Config snippet to enable a block with a profile (in this case the help block for the bartik theme in the standard profile):

diff --git a/core/profiles/standard/config/plugin.core.block.bartik.help.yml b/core/profiles/standard/config/plugin.core.block.bartik.help.yml
new file mode 100644
index 0000000..a5c4467
--- /dev/null
+++ b/core/profiles/standard/config/plugin.core.block.bartik.help.yml
@@ -0,0 +1,18 @@
+id: system_help_block
+status: '1'
+cache: '-1'
+visibility:
+  path:
+    visibility: '0'
+    pages: ''
+  role:
+    roles: {  }
+  node_type:
+    types:
+      article: '0'
+      page: '0'
+  visibility__active_tab: edit-visibility-path
+subject: ''
+module: system
+region: help
+weight: '0'
Gábor Hojtsy’s picture

Status:Needs review» Needs work
Gábor Hojtsy’s picture

StatusFileSize
new28.25 KB

@EclipseGC said in IRC we might not need to introduce a new region. Well, currently, the header region in bartik for example is on the right side of the header while logo and slogan and site name (compounded!) are on the left side:

TopRegions.png

Since we don't have any means to position (float) blocks in regions as we wish, I don't think an overall header region works. Unless we make the header region 100% wide, place the logo at the first place. Then introduce a "site name and slogan" compound block (not as two separate blocks) and place that by the side of the logo. So all blocks in the header region would be left floated in bartik.

Finally, I just realised we need to get rid of the theme settings for logo, site name and slogan display both on the global and per-theme level as well. That would need an upgrade path, however blocks have no upgrade path yet, so that upgrade path might need to go into #1871700: Provide an upgrade path to the new Block architecture from Drupal 7 eventually, not in this patch.

Gábor Hojtsy’s picture

IRC review from EclipseGC on regions:

EclipseGc: GaborHojtsy: you how to on that look right to me, I think the new regions is debatable, but it's also the quickest way forward for some things. (primary and secondary menus for instance)
EclipseGc: GaborHojtsy: and your proposed way to do this w/o introducing new regions is exactly what I was expecting. Theme's are going to provide their own version of block or hook_block_view_alter() them, in which they should ultimately be able to add/change the css, and weight these things appropriately
EclipseGc: GaborHojtsy: but as I said, I'm also not opposed to adding new regions here

On the theme settings:

EclipseGc: GaborHojtsy: ok, so totally ++ on the removing of those settings

dcrocks’s picture

It looks like there are 3 things to be done: (1) make these elements (configurable)blocks, (2) determine which of the 3 core themes need to support these elements as blocks and modify appropriately, and (3) modify install so these blocks are configured into the core themes that will use them by default. Does all this have to be done in this issue? Shouldn't modifying the core themes to support these elements as blocks be a separate issue for each theme?

EclipseGc’s picture

4.) We may have some caching issues depending on how themes handle this.

Eclipse

Gábor Hojtsy’s picture

@dcrocks: well, unless we modify themes, the blocks will be there but not used/useable for anything. Not sure what value does a patch committed on that level of usefulness has => hard to argue to break it down to that much IMHO. If we consider theme changes should be their own followups, then the extent of the change is already in the above proposed patch.

dcrocks’s picture

Not saying to postpone anything. I would think a theme related follow-up would be immediate. It just seemed to be a clearer discussion and, almost, a way to self document the change. And, as soon as these blocks are available, I expect people would use them. I would/will.

Gábor Hojtsy’s picture

@dcrocks: as demonstrated on the screenshot in #13, it is not possible to place a logo or site name block where the logo or site name is in bartik, so using it as soon as available (for its main purpose) is not possible without modifying the theme.

dcrocks’s picture

Does not the existing code in bartik work after these elements are made into blocks? Are the variables removed? What happens to seven and stark and the core system templates? I didn't think this patch broke any existing themes.

Gábor Hojtsy’s picture

@dcrocks: did I say that? AFAIS all I said is this does not make the blocks usable because the themes provide no place for them to be put in their primary role, at least there is no region there to use.

dcrocks’s picture

The existing patch only adds the blocks. If it is also going to modify bartik or any other core code, perhaps the issue title should be changed

andypost’s picture

Status:Needs work» Needs review

#9: site-elements-as-blocks-7.patch queued for re-testing.

dcrocks’s picture

StatusFileSize
new87.2 KB

Cannot #9 be committed as it stands? I tested it in 8.x-dev dated 1/18 with no ill affects. Committing it had no affect on existing core themes. With it in people can start using it for 8.x themes. A new issue to have bartik take advantage of this could be started separately. In the attached image I assigned them to sidebar 2 in bartik but made absolutely no changes to bartik, just to see what it would look like.

ps. I had to enable the layout module to see these. Why aren't they built as system plugins instead of layout plugins? They are system data aren't they?

dcrocks’s picture

Actually, they have to be in a module enabled during standard install, else they won't be available to bartik at install. System module seems the logical place.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

The blocks indeed seem to be more logical to be exposed by system module where the related settings are. Definitely not in layout module.

dcrocks’s picture

Before another patch is created, does it sound reasonable that some default styling be done on these blocks in core? They are not ugly, even in stark, but some standard default appearance could be given to them.

sun’s picture

  1. Can we introduce a 'branding' region as one of the default regions? Seven implements that already (in HTML markup, not as region). There's almost no theme that hasn't a branding region (except, Seven, actually, *g*), so it would be sane to introduce that as a default region.
  2. Styling should be left to individual themes. If we want to discuss "block styles", then we'll quickly start to duplicate the discussion that happened in #503810: Convert primary and secondary menus to blocks already — essentially putting the basics of Skinr into core; (1) Registering class names as styles, and (2) allowing to assign styles to blocks. We should probably file a dedicated issue for that topic, since that would be the complementing counter-part to layouts. (Layouts control placement, Styles control appearance.)
Gábor Hojtsy’s picture

@sun: branding region sounds nice.

Currently there are styles for a site name (bigger font) or slogan (italics in some cases), so not sure if you are suggesting to remove all those altogether (eg. you could only place the site name in the regular page font size/type and not reproduce the above screenshot where it is with a larger/different(?) type). That sounds like a regression.

xjm’s picture

Category:feature» task
sdboyer’s picture

just noting that this is a blocker for SCOTCH, and the work i'm currently doing in our princess branch.

it may be that this is easier to finish up in princess directly because we don't have to deal with some of the architectural awkwardness (like special regions). if that's the case, i may just pull the base work in there and go forward with it.

webchick’s picture

This is another one of those issues that it'd be great to have as a patch separate from the princess branch, because it has value outside of SCOTCH.

xjm’s picture

Priority:Normal» Major
oadaeh’s picture

Status:Needs work» Needs review
Issue tags:-Blocks-Layouts, -B&L

#9: site-elements-as-blocks-7.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Blocks-Layouts, +B&L

The last submitted patch, site-elements-as-blocks-7.patch, failed testing.

lokapujya’s picture

Status:Needs work» Needs review
StatusFileSize
new3.02 KB
Test request sent.
[ View ]

Portland Code Sprint is wrapping up. Here is a quick port of the patch in #9 to D8, moving plugin to system.

hosef’s picture

Status:Needs review» Needs work

The patch looks ok, however the 'subject' property in the class anotation has been renamed to 'admin_label'.

Also, it looks like the test bot is having problems.

EDIT: We should probably alter Bartik to use these blocks instead of just printing the items out.

oadaeh’s picture

I think the test bot problems are from the overload at the sprint. I suspect it will be fine the next time it runs.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteLogo.php
@@ -0,0 +1,34 @@
+ *   subject = @Translation("Site Logo"),
+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteName.php
@@ -0,0 +1,30 @@
+ *   subject = @Translation("Site Name"),
+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteSlogan.php
@@ -0,0 +1,35 @@
+ *   subject = @Translation("Site Slogan"),

These are the lines that should have "subject" replaced with "admin_label".

lokapujya’s picture

Status:Needs work» Needs review
StatusFileSize
new3.04 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]

"subject" changed to "admin_label"

lokapujya’s picture

Status:Needs review» Needs work

When trying to place the block:

Fatal error: Class Drupal\system\Plugin\Block\PageSiteLogo contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\block\BlockBase::blockBuild) in /Users/jfallon/Sites/d8/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteLogo.php on line 20

So, there is more work to do.

aspilicious’s picture

Why do u use children and not #markup?

lokapujya’s picture

Status:Needs work» Needs review
StatusFileSize
new2.36 KB
new3.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,901 pass(es).
[ View ]

Changes needed so that placing blocks to Bartik header region does not generate errors.

lokapujya’s picture

Issue summary:View changes

Updated issue summary.

lokapujya’s picture

aspilicious: Can you make a case for #markup over #children?

The goal in the latest patch was just to make the patch from comment #9 work in D8.

Status:Needs review» Needs work

The last submitted patch, 1053648-site-elements-as-blocks-8_1.patch, failed testing.

hosef’s picture

I have been working on #2005546: Modify regions to support Convert site elements (site name, slogan, site logo) into blocks and I think it would be easier to have the site name and the site slogan handled by the same block.

aspilicious’s picture

They have to be seperate blocks imo

hosef’s picture

I was talking in IRC with @lokapujya about how to address the issue of getting the site name and site slogan to line up the way they did before. We see a few solutions:
The first is to make them into a single block, however, this would limit the amount of layout customization one has.
The second would be to add even more regions to the header. If we go with the add more regions route we could end up with 6+ regions in Bartilk's header area.
A third option would be to just palce the blocks with CSS, however simple styling can easily be broken by just moving some blocks around or having a really long site name, thus requiring dozens of selectors to try and catch all of the likely scenarios. The layout module looks like it might be a good solution for this at some point, however it is still far from complete.
In short, we still don't know what the best option is(not helpful, I know :P).
If anyone has any ideas that would be great.

LewisNyman’s picture

I don't think we can make any assumptions about site design, what if I want my slogan in the footer?

The theme should decide how different blocks layout within a region. It's a tricky problem to deal with in core or contrib themes but it's no different to the situation now in D7.

lokapujya’s picture

Status:Needs work» Needs review
lokapujya’s picture

Issue summary:View changes

Adding sub-task for Bartik theme.

lokapujya’s picture

Issue summary:View changes

Simplifying scope of this issue.

hosef’s picture

The text in the site name block needs to be a link to the home page.

Status:Needs review» Needs work

The last submitted patch, 1053648-site-elements-as-blocks-8_1.patch, failed testing.

oadaeh’s picture

Issue tags:-B&L

Removing the unnecessary B&L tag.

jibran’s picture

Status:Needs work» Needs review
Jeff Burnz’s picture

I think we're are missing a really valuable point in Drupal 8 - that a block can have as many instances as you want.

So, we could do it all in one block - logo, site name and slogan. Have configuration in the block that allows you to choose which element you want 'in this instance of the block'.

We could also rather easily write classes into the block attributes that tell us what bits are being used (just a suggestion, personally I would do this).

Get the best of both worlds, for Bartik and themes that do not want to laden themselves down with screeds of CSS "what ifs" get the advantage of being able to lock down the design and save site builders from (easily) breaking their theme.

For those cases where these things need to be reordered or have some edge case requirement like slogan in the footer Drupal supports that capability already. Just use another instance of the block and turn off what you do not want.

This does not really take anything away from the flexibility but allows us themers to actually produce designs that will work robustly and not "way to easily" broken by the site builder (and who may not even understand why or how their theme/site got broken).

I think this could be a workable solution. Thoughts?

Gábor Hojtsy’s picture

Sounds like a good approach to me.

Jeff Burnz’s picture

I think its a good approach also but I have to actually code this up and see how hard this is the theme, I mean get inside {{ content }} in the block (whether in a template/preprocess/hook_theme_prepare/alter etc) and reorder things in code, as long as thats reasonably easy I see not too many other issues with this approach.

Jeff Burnz’s picture

StatusFileSize
new3.75 KB
PASSED: [[SimpleTest]]: [MySQL] 63,308 pass(es).
[ View ]

OK, heres a proof of concept patch for discussion and review.

This harks back to #28/29 sort of but instead of providing a branding region we provide a branding block and allow site builders to toggle the elements they want in that particular block instance.

As discussed above this has the rather nice advantage of locking down the source order pretty tightly, which is either good or bad depending on how you want to look at this. I have approached this so you can change the order in code pretty easily, which you always had to do before anyway, just a different bit of code: e.g.: $variables['content']['branding']['sitename']['#weight'] = -1; would move the site name to the top.

Caveat, this is the first patch I ever wrote for D8 and the first one in years, so expect that I am out of touch and probably missed some docs, this is for discussion regarding the approach.

dcrocks’s picture

StatusFileSize
new61.29 KB

Tried it out, seemed to work fine. But, would prefer separate blocks for each content type just to make it easier to identify what is actually being displayed by each instance. As you can see in
block table
There is no way to tell, or change, on the block admin page, what each instance is actually displaying. Contextual links are usable for seeing and displaying block instance configs but it is inconvenient not to be able to do the same on the block page.
PS. These instances are indeed displaying different content.

Jeff Burnz’s picture

I think strictly speaking that might even be outside the scope of this issue, e.g. a broader UX issue that may or may not have been discussed already, mainly because this affects not only this block but views blocks, custom blocks etc. I'd say if we get a directive to include custom labels in block admin to show config values then we should do it. I would defer to the UX peeps on that particular issue. Basically the solution to the admin labelling issue is not to have these as separate blocks but to use CMI (I have not delved deep into BlockBase as yet).

tim.plunkett’s picture

Issue summary:View changes

Formatting Change.

Jeff Burnz’s picture

Jeff Burnz’s picture

andypost’s picture

Good idea! seems this branding needs some theming and css to actually move forward

The last submitted patch, 57: 1053648-site-elements-as-blocks-8_2.patch, failed testing.

Jeff Burnz’s picture

dcrocks’s picture

Issue summary:View changes
StatusFileSize
new62.87 KB

I tried the patch on a fresh clone of D8. The attached image shows sample output.It looks like it might be difficult to theme but I don't see how you can add theming thru this patch as you don't know where or how each instance will be used. If you look at a sample of the generated output

<div id="sidebar-second" class="column sidebar"><aside class="section">
<div class="region region-sidebar-second">
<div class="block block-system contextual-region" id="block-sitebranding-2">
  <h2>Site Branding B</h2>
  <div data-contextual-id="block:block=sitebranding_2:"></div>
  <div class="content">
  <p class="site-slogan">Look Here</p>
  </div>
  </div>
  <div class="block block-system contextual-region" id="block-sitebranding">
  <h2>Site Branding A</h2>
  <div data-contextual-id="block:block=sitebranding:"></div>
  <div class="content">
  <h1 class="site-name"><a href="/~rocks/drupal8/index.php/" rel="home" title="Home">Drupal8</a></h1>
  </div>
  </div>
  </div>

You can see there is not much help for theming.

dsnopek’s picture

@dcrocks: Yeah, it looks like there should be at least a class name on the block that identifies the block delta, now that the same block can be used multiple times..

Jeff Burnz’s picture

#66, #67 there is a unique ID added by Drupal core, if core wants to add a unique class per instance (which I support) that is a separate issue.

One think I mentioned earlier in the thread (#54) was adding configuration type classes, aka so you know what is active in the block, e.g. .has-logo, .has-sitename sort of thing. Thats something I have generally done on branding type wrappers in my themes, it can be useful, but also kind of edge casey, maybe in this system it would be more important since you are, in essence, having multiple branding type elements (blocks).

Basically we'd have to get that info from config/add the classes in preprocess, not sure that is ideal for core from a general performance perspective, I personally think that those classes could be edge cases and perhaps it's better to leave that to the theme to implement - at least that seems to be the general direction core is heading in - aka a far more minimal approach to markup out of the box, which is why this patch is conservative and does not include special casing for classes.

dsnopek’s picture

@Jeff Burnz: I wasn't suggesting a unique class, but one that identified the block plugin. There are multiple blocks on the page from the same plugin, but no class to tie them together (other than block-system), so you can't apply the same CSS to them all. Does that make sense? Sorry, if I've been unclear.

I'm picturing something like:

<div class="block block-system block-system-branding-block contextual-region" id="block-sitebranding-2">
</div>
...
<div class="block block-system block-system-branding-block contextual-region" id="block-sitebranding-3">
</div>

I added the "block-system-branding-block" (based on the plugin ID of "system_branding_block") so that you know that each of those blocks represent the same block plugin.

But, yes, I agree this is a seperate issue. :-)

Jeff Burnz’s picture

#69 ah right, there is an issue for that, I'll dig around and see if I can find it, we argued/discussed this at length some time ago which stalemated over the block- prefix.

Jeff Burnz’s picture

This is the issue I think is worth looking at regarding plugin class: #1896098: Add a plugin class to the blocks to identify instances in css

Jeff Burnz’s picture

StatusFileSize
new3.81 KB
PASSED: [[SimpleTest]]: [MySQL] 63,321 pass(es).
[ View ]

Site name should be check plained, so now uses String::checkPlain()
BEM'd the classes.

One thing we should note is that this patch does not account for the Toggle theme settings, which became rather ambiguous - so Site name, Slogan toggles would be removed, and Logo toggle modified to account for there being no visibility toggle within the theme settings, just a file field to override the themes default logo. Should we add/do this here, or not?

dsnopek’s picture

@Jeff Burnz: Thanks for finding the issue! :-)

andypost’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,115 @@
    +    $site_config = \Drupal::config('system.site');

    Config should be properly injected

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,115 @@
    +      $slogan = filter_xss_admin($site_config->get('slogan'));

    Use Xss::filterAdmin()

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,115 @@
    +        $image = '<img src="' . $logo['url'] . '" alt="' . t('Home') . '" />';
    +        $build['branding']['logo'] = array(
    +          '#markup' => l($image, '', array('html' => TRUE, 'attributes' => array('rel' => 'home', 'class' => array('logo'), 'title' => t('Home')))),
    ...
    +        '#markup' => l($sitename, '', array('attributes' => array('rel' => 'home', 'title' => t('Home')))),
    ...
    +        '#suffix' => '</h1>',
    ...
    +        '#prefix' => '<h1 class="site__name">',
    ...
    +          '#markup' => $slogan,
    +          '#prefix' => '<p class="site__slogan">',
    +          '#suffix' => '</p>',

    Is there a way to use proper render array?

Jeff Burnz’s picture

#74

"Config should be properly injected", not sure what is meant by this, I copied whats in theme.inc preprocess for now, is there an example I can follow?

"proper render array" - yeah, I sort of get what you mean, this stuff always seems so confusing to me, what to do where etc. I assume you mean don't call theme l() here and do a full proper render array for the whole lot, if so, yes, I agree.

joelpittet’s picture

This may help with the "proper render array" thing...

    
        '#type' => 'link',
        '#title' => $sitename,
        '#href' => '<front>',
        '#attributes' => array('rel' => 'home', 'title' => t('Home')),

Config maybe should be injected but none fo the base classes seem to have it already injected, so from a pain in the ass point of view... this is very likely how it will be done in the wild:) However, translation and the t() function are already injected for you so you can use $this->t().

There is a $this->configuration, but I don't think that is the same, that looks like the block configuration and not the config factory from CMI.

I'm trying to formulate a plan in my head to avoid markup being used in #prefix/#suffix because that gets abused too much but so far I've got nothing, so until a brilliant idea come about, carry on:)

joelpittet’s picture

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
@@ -0,0 +1,115 @@
+    $build = array();
+    $site_config = \Drupal::config('system.site');
+    // Logo
+    if ($this->configuration['logo'] == TRUE) {
+      $logo = theme_get_setting('logo');
+      if (!empty($logo)) {
+        $image = '<img src="' . $logo['url'] . '" alt="' . t('Home') . '" />';
+        $build['branding']['logo'] = array(
+          '#markup' => l($image, '', array('html' => TRUE, 'attributes' => array('rel' => 'home', 'class' => array('logo'), 'title' => t('Home')))),
+          '#weight' => 0,
+        );
+      }
+    }
+    // Site name
+    if ($this->configuration['sitename'] == TRUE) {
+      $sitename = String::checkPlain($site_config->get('name'));
+      $build['branding']['sitename'] = array(
+        '#markup' => l($sitename, '', array('attributes' => array('rel' => 'home', 'title' => t('Home')))),
+        '#prefix' => '<h1 class="site__name">',
+        '#suffix' => '</h1>',
+        '#weight' => 1,
+      );
+    }
+    // Site slogan
+    if ($this->configuration['siteslogan'] == TRUE) {
+      $slogan = filter_xss_admin($site_config->get('slogan'));
+      if (!empty($slogan)) {
+        $build['branding']['siteslogan'] = array(
+          '#markup' => $slogan,
+          '#prefix' => '<p class="site__slogan">',
+          '#suffix' => '</p>',
+          '#weight' => 2,
+        );
+      }
+    }

If this is to be themed, it should likely be a twig template.
core/modules/system/templates/branding.html.twig Or something along those lines.

array(
  '#theme' => 'branding',
  '#use_logo' = $this->configuration['logo'],
  '#logo' = $logo,
  etc...
)
  {% if use_logo %}
    <a href="{{ site_url }}" ...><img src="{{ logo.url }}" alt="{{ 'Home'|t }}" /></a>
  {% endif %}
  etc.

Thoughts?

mdrummond’s picture

StatusFileSize
new5.99 KB
PASSED: [[SimpleTest]]: [MySQL] 64,346 pass(es).
[ View ]
new8.25 KB

Worked with timplunkett and joelpittet in IRC to create a revised version of this patch that addresses the above concerns, and which uses a twig template, which is much cleaner. Please note that we changed the class for the logo to 'site__logo' to match the pattern of the other branding elements.

joelpittet’s picture

Issue tags:+Twig, +focus

Tagging.

LewisNyman’s picture

+++ b/core/modules/system/templates/branding.html.twig
@@ -0,0 +1,22 @@
+  <a href="{{ url('<front>') }}" rel="home" class="site__logo" title="{{ 'Home'|t }}"><img src="{{ site_logo.url }}" alt="{{ 'Home'|t }}" /></a>
...
+  <h1 class="site__name"><a href="{{ url('<front>') }}" rel="home" title="{{ 'Home'|t }}">{{ site_name|e|t }}</a></h1>
...
+  <p class="site__slogan">{{ site_slogan|t }}</p>

I'm not able to review the rest of the patch but the use of double underscore's seem a little off. Our CSS standards specify the usage to:

define a component’s elements explicitly, prefixing them with the component’s name followed by two underscores

Which means a class like site__logo would mean there is a component called site that contains this element.

My suggestion would be just to stick with site-logo

Jeff Burnz’s picture

Great, this is where it needed to go, the template is a vast improvement as is the rest of the patch. Class names should probably be changed as Lewis points out, my bad, I lumped in my own approach here (my earlier patch was cut from my own test site where I use this form of BEM, but it's not cores approach precisely).

I was not able to uncheck by default the "Display title" toggle in my hacking on earlier patches, is this actually possible, I would think no one wants to show a title?

mdrummond’s picture

I tested the patch on simplytest.me to see what you meant by the display toggle. That toggles the title of the block on and off: by default, it has the title Site branding.

I agree, I would be hard pressed to think of a reason why somebody would want to create a block for site branding elements with the title showing. I'm not familiar enough with the new block plugin system to know how to toggle that off by default, but maybe somebody else can point the way.

Changing the class names is very doable. Change them to "site-name", "site-logo" and "site-slogan"?

Just to clarify the roadmap, this issue is specifically to provide the site branding elements as an optional block, correct? Once that is in, would we create these sub-issues as followups?

  • Create a branding region for themes, which would include the site branding block by default
  • Remove the site branding elements from the page template in favor of the branding region
  • Deprecate the site branding variables and remove the variables to turn them on and off on the appearance page

If that's the eventual roadmap, I would imagine we'd need to follow a similar process for menu items (#1869476: Convert global menus (primary links, secondary links) into blocks) and titles, tabs, actions and messages (#507488: Convert page elements (title, local tasks, actions) into blocks).

mdrummond’s picture

It looks like the display title toggle for blocks was introduced in this issue:

#1875260: Make the block title required and allow it to be hidden

As best as I can tell, we need to add something similar to this for blockForm:

    $form['label_display']['#default_value' ] => FALSE,
joelpittet’s picture

@mdrummond re: #83 That looks like the right answer but it won't work because the configuration form is unioning™ the blockForm() onto it's array so it won't override BlockBase::buildConfigurationForm. But I do believe you can change it through $this->configuration['label_display']   Though I'm not sure where to put it best... constructor would be my guess.

lokapujya’s picture

Tested the suggestion in #84. It works, and seems like the right place to put defaults to me.

Side note: Bartik is currently hiding all block titles in the header region via CSS.

mdrummond’s picture

StatusFileSize
new6.01 KB
PASSED: [[SimpleTest]]: [MySQL] 64,257 pass(es).
[ View ]
new1.54 KB

Two things in this patch and interdiff:

1) Changed the site__ classes to site- classes in the twig template.

2) Toggled the block title display to off by default. After a lot of tries, this was surprisingly easy. Just needed to add 'label_display' => FALSE to the defaultConfiguration array. The other magic that makes that work is inherited from BlockBase, at least as far as I can tell. Hopefully this is the right approach. It does seem to work fine, from several tests I did.

Go go green testbot!

mdrummond’s picture

Would be good to get some extra eyes on this patch to make sure that this approach looks doable. If so, next items on the to-do list are:

  1. Create branding region in Bartik. (Is this needed in Seven, which doesn't have branding? Stark?)
  2. Load site branding block into branding region on install.
  3. Ensure that branding region and branding block have same CSS as previous branding elements.
  4. Remove site branding elements from page template in Bartik (check if removal needed in Seven and Stark too).
  5. Change appearance page and config to remove ability to turn site branding elements on and off.
  6. Make any necessary changes to tests. If possible name the new region branding rather than the current name-and-slogan id.

Any thoughts on if those should be a separate follow-up issue to this one, or if we need a mega-patch that does all of those things?

Personally, I'd love to start by just getting the ability to add site branding elements anywhere on the site. I'd hate to see that ability get significantly delayed by the need to get all of the above items completed first.

Les Lim’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,137 @@
    +use Drupal\block\Annotation\Block;
    +use Drupal\Core\Annotation\Translation;

    These aren't necessary.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,137 @@
    +use Drupal\Core\Config\ConfigFactory;

    Typehint with the interface whenever possible; change this to Drupal\Core\Config\ConfigFactoryInterface here and in corresponding places below.

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,137 @@
    +   * @param \Drupal\Core\Config\ConfigFactory $configFactory
    +   *   The factory for configuration objects.
    +   */
    +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, ConfigFactory $configFactory) {
    +    parent::__construct($configuration, $plugin_id, $plugin_definition);
    +    $this->configFactory = $configFactory;
    +  }
    +

    Object properties are lowerCamelCase, but variables are lower_snake_case, I believe: so $this->configFactory = $config_factory;

  4. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,137 @@
    +    $build['#site_slogan'] = XSS::filterAdmin($site_config->get('slogan'));

    "Xss" is the actual class name. (PHP doesn't care about class name casing, but we should!)

Les Lim’s picture

  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,137 @@
    +   * Overrides \Drupal\block\BlockBase::blockForm().

    "Implements" and "Overrides" docblocks that don't add any more documentation should just be {@inheritdoc} instead:
    /**
    * {@inheritdoc}
    */

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,137 @@
    +  /**
    +   * Overrides \Drupal\block\BlockBase::blockSubmit().
    +   */

    @{inheritdoc}

mdrummond’s picture

StatusFileSize
new5.91 KB
PASSED: [[SimpleTest]]: [MySQL] 64,315 pass(es).
[ View ]
new2.8 KB

Here's a patch that addresses the concerns in #88 and #89.

Tried it out, seems to still work fine.

Jeff Burnz’s picture

#87 I'd like to see this land, then follow up:

Agree with no more delays, we need to agree on an approach which plausibly gives us a blueprint for the other elements-as-blocks patches also, to me this is looking very good for users and themers, being able to lock down that source order is a big win, negating much hijinks required in CSS.

mdrummond’s picture

I can start working on items 1-4 later today, starting with the work done in #2005546: Modify regions to support Convert site elements (site name, slogan, site logo) into blocks. In the meantime, it would be great to get some final looks at the patch in #90. If it's looking good, and somebody else wants to test this out, maybe this we can get this issue to RTBC?

dcrocks’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new100.09 KB

Cloned D8, invoked patch, then went thru install. No problems. Created several 'site' blocks, moved them around, deleted a 'site' block, all with no problems. Think it's rtbc

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

We should actually try using this, if not in actual usage, then just in a test.

Additionally,

  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,135 @@
    +  protected $config_factory;
    ...
    +    $this->configFactory = $config_factory;

    It should be protected $configFactory;
    The seeming mismatch of $this->configFactory = $config_factory; is correct.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,135 @@
    +      '#title' => $this->t('Toggle branding elements'),
    +      '#description' => t('Choose which branding elements you want to show in this block instance.'),

    Half are using t(), the other half $this->t(). Please use $this->t() everywhere.

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,135 @@
    +    // Logo
    ...
    +    // Site name
    ...
    +    // Site slogan

    Either make these full sentences, or just remove them.

mdrummond’s picture

I'll take care of making those three changes tonight. I haven't written tests before so I might need to find some assistance with that.

The other thing I was thinking about was that we may want to get the markup/css to match more closely with the pattern in system's page.html.twig. One big thing is to change the wrapper for the site name to strong instead of h1. We don't need h1's all over the page.

When we do the region setup, we could possibly add a feature to the block so that if the block was in the branding region, and if the page didn't already have another title with an h1, then change the wrapper for the site name to h1 rather than strong.

If a theme wants to have the main branding for the page in a region other than branding, with an h1 for the site name, then that could be handled within that's themes templates.

One other discrepancy I noticed was that by default, the branding elements use class names, while in Bartik the branding elements use id attributes for styling hooks. For the generic block elements, we should probably use class names, since there could be multiple copies of the block. For Bartik maybe there's a way to put an override on the branding twig file to use id attributes if the block is within the branding region?

I'm not sure why Bartik uses id attributes for styling hooks rather than class names but changing that is probably beyond the scope of these issues, I'd assume.

mdrummond’s picture

StatusFileSize
new3.42 KB
new6.32 KB
PASSED: [[SimpleTest]]: [MySQL] 64,314 pass(es).
[ View ]

This patch addresses the suggestions Tim made in #94.

I also changed the markup to match the pattern in system's page.html.twig file. That should make it easier to ensure other blocks behave the same as the default and will make the branding region issue a bit easier.

I also tweaked the logic in the twig file and clarified the docblock. The docblock now explains that the use_ variables come from the block settings, not the appearance page. Eventually we'll probably want to remove the toggles for site name, site logo and site slogan from the appearance page, but that's a separate issue.

The site name is required, but the site logo and site slogan are not. While there is a default site logo of the Druplicon, there is no default site slogan. So this now tests both if the block asks to use the slogan and if a slogan actually exists. page.html.twig just tests if the site name, site logo and site slogan exist for those parts of the markup.

We could test if the site name exists, but since it's a required field, I'd be hard pressed to think of a situation where that wouldn't be set.

It might be unlikely for a site logo to not exist, but it is possible, so makes sense to test for that I think.

mdrummond’s picture

Status:Needs work» Needs review

Go testbot go!

mdrummond’s picture

Oh one more note on the tests. In talking to Joel, my understanding is that to test if the site branding block works we would need to have an instance of the block be created in a region by default. Since the plan is to create the branding region in a separate issue, it might be hard to set up the tests on these site branding blocks right now. However, I don't know a lot about tests, so I could be completely wrong on that.

Would some additional manual testing be sufficient to commit this?

joelpittet’s picture

re: tests: that's the only way I can think of testing this. Not a testing expert by any means(copy paste and manipulate existing tests).

Les Lim’s picture

Issue tags:-Needs tests
StatusFileSize
new10.58 KB
PASSED: [[SimpleTest]]: [MySQL] 64,401 pass(es).
[ View ]
new4.26 KB

I'm not a testing expert either, but I don't think we need to test that the block displays in a particular region. The whole point of this patch is that we don't want to enforce the location of the branding elements.

I think we do need to test that the block settings for showing/hiding each branding component work as expected, though. Here's that.

mdrummond’s picture

The site name off test will likely fail, as we don't have an if for an empty site name since that is required.

We should also probably test the use_site_name, use_site_slogan and use_site_logo settings which are different. You might also want to test site_name exists but use_site_slogan off to see if the name-and-slogan exists.

There are probably a ton of combinations for each of these features too.

Les Lim’s picture

This actually is testing use_site_logo, etc. The configuration key for that in the patch is 'site_logo'; you're just representing that at build time as 'use_site_logo'.

it might be a good idea to tweak the patch so that the configuration options are actually named 'use_site_*' instead of just 'site_*'. Less ambiguous that way.

Jeff Burnz’s picture

+++ b/core/modules/system/templates/branding.html.twig
@@ -0,0 +1,40 @@
+{% if use_site_logo and site_logo %}

I haven't reviewed the rest of the patch but this seems a bit awkward, is it really necessary in the template (slogan also)?

mdrummond’s picture

Les: That's probably a good idea to switch the config options to use_site. Definitely more accurate that way. I can update that tonight.

Jeff: Yes, I believe that's necessary. We have two possible situations. One, somebody could use the block setting to not display the site slogan. Two, somebody could not set a site slogan on the site information page. We should only show the slogan if the block is set to display the slogan and there is actually a slogan to display.

Cottser’s picture

Status:Needs review» Needs work
  1. +++ b/core/modules/system/templates/branding.html.twig
    @@ -0,0 +1,40 @@
    +{% if use_site_logo or (use_site_slogan and site_slogan) %}
    +
    +    <div class="name-and-slogan">

    We should outdent the div.name-and-slogan by 2 spaces, that should fix the indentation here. I'm also not sure why this condition needs to care about use_site_logo, seems like that should be use_site_name.

  2. +++ b/core/modules/system/templates/branding.html.twig
    @@ -0,0 +1,40 @@
    +    </div>{# /.name-and-slogan #}

    I know we have some of these in core already but that was just keeping them in the process of converting from .tpl.php. I don't think we should add any new ones because there's no standard saying we should and most folks I've talked to are against them in the first place.

mdrummond’s picture

{% if use_site_logo or (use_site_slogan and site_slogan) %}

That is a typo on my part. Indeed, it should be use_site_name.

I'll take care of the outdenting and comment removal tonight.

LewisNyman’s picture

Couldn't we do something like this?

if($this->configuration['site_logo']) {
  $build['#site_logo'] = theme_get_setting('logo');
}

if($this->configuration['site_name']) {
  $build['#site_name'] = $site_config->get('name');
}

if($this->configuration['site_slogan']) {
  $build['#site_slogan'] = Xss::filterAdmin($site_config->get('slogan'));
}

I tried but it always broke...

Also, I think the name-and-slogan div makes no sense when we can place the names and slogans in separate blocks using multiple instances. I could have two name-and-slogan divs on the page where one only includes the name and one only includes the slogan. Do we even need a div to wrap both?

Les Lim’s picture

re: #107, I just had a chat with Cottser about this very thing:

9:57 Les_Lim instead of "use_site_slogan" in the twig template, is it more appropriate to do the conditional logic in the build() method?
9:57 Les_Lim or should the twig template know what the block settings were?
9:57 Cottser Les_Lim: could go either way. I think having display logic in Twig templates is perfectly fine
9:57 Cottser and I don't think we should be scared of logic in templates
9:58 Les_Lim k
9:58 Cottser probably better to expose that to people theming this stuff I'm thinking.
9:59 Cottser Les_Lim: more control via the templates that way. you could make the template ignore the UI settings if that's the way you feel :)
9:59 Cottser but I could understand doing it either way.

I'm fairly convinced by that; we shouldn't make judgments about what information the template doesn't need. There could very well be legitimate reasons for the template to have the logo or slogan available, even if it's set not to display.

mdrummond’s picture

Lewis:

The name-and-slogan div is what's currently in the system page template and in Bartik. I'm just trying to match what already exists so that we stay consistent. Those existing templates have the same logic: there is a name-and-slogan wrapper, even if there is only a name or a slogan.

I assume it is set up that way to allow for consistent styling for themers. In Bartik the name and slogan float to the right of the logo: I'd have to spin up a test site, but I'd bet a dollar that the floating is done on the name-and-slogan div rather than on the name or slogan elements, and that's why this markup pattern exists.

Getting rid of the wrapper div might not be a bad idea, but I'd suggest doing that as a separate issue once we get the ability to add branding blocks and then complete the next issue, which is replacing the current branding markup with a branding region and default branding block.

I could go either way on having the display logic in the build step or the template, but giving more options to themers in a twig template rather than an object oriented plugin is probably a good thing.

joelpittet’s picture

@LewisNyman the use_* prefix is there to represent the checkbox boolean value. It has no representation of if those values exist or not. I'd prefer to keep a one to one mapping of those variables. The name could change to is_* if that is more consistent with other booleans, but I thought use_* would be more appropriate. You may need those variables like sitename/slogan for page titles and other areas on the site, so the checkboxes seem like a good approach and place to put them on the block that is using them.

I'd have to agree with @Les lim

There could very well be legitimate reasons for the template to have the logo or slogan available, even if it's set not to display.

.

We don't necessarily even need to check that the image, site name or slogan exist to display their markup, that may be too much hand holding and people could just look at the markup to realize "Duh!, I forgot to add the logo, that why the src="" is blank." @mdrummond just added those in there to try to map close to what we have already to ease this patch into committal.

Cottser’s picture

Issue summary:View changes

Re #109 I tend to think that if we're doing multiple issues anyway to solve this in full we can add a template override in Bartik later if we need/want to preserve the name-and-slogan wrapper pattern in Bartik. IMO it would be nice if this block's markup was more generic and didn't make assumptions about patterns or use cases.

mdrummond’s picture

I'll have to check but I'm pretty sure the name-and-slogan pattern is in system as well as Bartik.

Cottser’s picture

You're right, so changing that would probably be a dream markup thing then if anything (#1980004: [meta] Creating Dream Markup). Carry on :)

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new12.68 KB
PASSED: [[SimpleTest]]: [MySQL] 64,439 pass(es).
[ View ]
new12.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1053648-100-114.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a patch to address all of the above issues.

1) Renames the config to use_site_name, etc. which should be clearer. The tests have been adjusted to get the settings for these config names.
2) Fixes the outdenting issue and the typo in branding.html.twig, as well as removing the comment on the closing div.
3) Some cleanup work in the tests to use similar naming conventions as in the rest of the patch. I also added test coverage to see if the name-and-slogan div is properly turned on and off based on the settings for using the site name and site slogan.

Hopefully this addresses the remaining issues and testbot goes green. Then on to manual testing!

Status:Needs review» Needs work

The last submitted patch, 114: interdiff-1053648-100-114.patch, failed testing.

The last submitted patch, 114: interdiff-1053648-100-114.patch, failed testing.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new12.68 KB
PASSED: [[SimpleTest]]: [MySQL] 64,447 pass(es).
[ View ]
new12.51 KB

Can we just pretend I didn't end the interdiff file name with .patch?

Thanks.

joelpittet’s picture

I was pretending till you brought it up:P

klonos’s picture

...we should really get used to hiding previous, obsolete patches/files when uploading new ones ;)

mdrummond’s picture

Anybody up for manually testing this?

Cottser’s picture

Status:Needs review» Needs work

I did some manual testing and everything checks out as far as functionality. Here's some nitpickery and docs suggestions, I love this patch!

  1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
    @@ -0,0 +1,116 @@
    + * Definition of Drupal\block\Tests\BlockSystemBrandingTest.

    This should be "Contains \Drupal\block…", Definition of without the leading backslash is an outdated standard.

  2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
    @@ -0,0 +1,116 @@
    +    $this->assertTrue(empty($site_name_element), 'The branding block site name was found.');
    +    $this->assertTrue(empty($site_slogan_element), 'The branding block slogan was disabled.');
    +
    +  }

    Super minor but we can lose the blank line at the end of this test method.

  3. +++ b/core/modules/system/templates/branding.html.twig
    @@ -0,0 +1,40 @@
    + * - site_logo: Logo for site.
    + * - site_name: Name for site.
    + * - site_slogan: Slogan for site.

    These might read better (or less terse) if they were just "Site logo.", but it also might make sense to indicate where these variables are configured. (Site information/theme settings).

  4. +++ b/core/modules/system/templates/branding.html.twig
    @@ -0,0 +1,40 @@
    +{% if use_site_logo and site_logo %}
    +  <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo">
    +    <img src="{{ site_logo.url }}" alt="{{ 'Home'|t }}" />
    +  </a>
    +{% endif %}
    +
    +{% if use_site_name or (use_site_slogan and site_slogan) %}
    +
    +  <div class="name-and-slogan">
    +
    +    {% if use_site_name %}

    Why does the name and slogan one have all the extra newlines? Seems like we don't need the newlines when we have indents. Just have a newline to separate the logo from the name and slogan.

Les Lim’s picture

What Cottser said, and:

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
@@ -0,0 +1,116 @@
+    $this->assertTrue(empty($site_name_slogan_element), 'The branding block name and slogan div was found.');
+    $this->assertTrue(empty($site_name_element), 'The branding block site name was found.');

The message doesn't match the condition in these tests; both of these lines are testing that those elements are disabled.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new12.79 KB
PASSED: [[SimpleTest]]: [MySQL] 64,402 pass(es).
[ View ]
new2.6 KB

Here's a patch to fix the issues in #122 and #123. Thanks for reviewing this!

mdrummond’s picture

Green! One step closer!

joelpittet’s picture

May be nice to have field help text to tell users where to set those variables for logo and site name/slogan?

mdrummond’s picture

StatusFileSize
new13.01 KB
PASSED: [[SimpleTest]]: [MySQL] 64,444 pass(es).
[ View ]
new1.95 KB

Here is a patch to add that help text. Also removed a blank line in the twig docblock between the two types of variables.

joelpittet’s picture

Minor things from reading through, otherwise I think this is RTBC.

  1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
    @@ -0,0 +1,115 @@
    + * Tests site branding block.

    Should probably read something like:
    * Provides testing for the branding block module functionality.

  2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
    @@ -0,0 +1,115 @@
    +    $site_name_slogan_element = $this->xpath('//div[@id="block-site-branding"]//div[@class="name-and-slogan"]');
    ...
    +    $this->assertTrue(!empty($site_name_slogan_element), 'The branding block name and slogan div was found.');

    Testing that the wrapper div is there doesn't do us any favours, probably can be scrapped in all instances.

mdrummond’s picture

StatusFileSize
new11.94 KB
PASSED: [[SimpleTest]]: [MySQL] 64,445 pass(es).
[ View ]
new5.1 KB
joelpittet’s picture

Status:Needs review» Reviewed & tested by the community

This has:

  • Adequate test coverage.
  • Been manually tested by a number of people.
  • Allows us to simplify our page.html.twig in the follow-up.
  • Provides much more flexibility for contrib to selectively hide and place branding elements within different layouts.
  • Keeps the markup inline with what is currently used in core

Feel free to disagree, if I missed anything.

Jeff Burnz’s picture

  1. +++ b/core/modules/system/templates/branding.html.twig
    @@ -0,0 +1,35 @@
    +{% endif %}

    Unnecessary new line?

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,135 @@
    +      '#description' => $this->t('Defined in appearance or theme settings.'),

    Capitlize Appearance? I.e. Defined in Appearance or theme settings.

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,135 @@
    +      '#description' => $this->t('Defined in site information settings.'),

    Capitalize Site? Same with Slogan description, i.e. Defined in Site information settings.

mdrummond’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new11.94 KB
PASSED: [[SimpleTest]]: [MySQL] 64,522 pass(es).
[ View ]
new2.53 KB

This addresses the concerns in #131. Changed the capitalization in branding.html.twig as well.

Would be great for somebody to look this over quick to get back to RTBC.

mdrummond’s picture

Passed if anybody wants to look this over again.

joelpittet’s picture

Thanks @Jeff Burnz, I'll let you re-RTBC if that fits the bill. Thanks for the space catch, I should've noticed.

joelpittet’s picture

Thanks @Jeff Burnz, I'll let you re-RTBC if that fits the bill. Thanks for the space catch, I should've noticed.

mdrummond’s picture

StatusFileSize
new11.57 KB
PASSED: [[SimpleTest]]: [MySQL] 64,605 pass(es).
[ View ]
new6.94 KB

Cottser provided some additional feedback in IRC. Changed some comments and also refactored xpaths for branding elements into variables in case those need to change in the future.

Cottser’s picture

Issue tags:-focus+sprint

Oops, 'sprint' is the right tag for http://drupaltwig.org/issues/focus :)

Go @mdrummond go! I think this is looking very very good.

Jeff Burnz’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me, those were very minor niggles.

I've spent quite some time theming with this patch and I find it hard to fault. I think some novice themers might get stumped by the template instance issue - meaning you have to theme at the block instance suggestion level to modify the markup per instance, however this conceptually little different to breaking out $content in a node template, just different template names. I mention this here as I think we should put some help along those lines into the change notice.

All in all I really like it, as a themer I can definitely work with this and I think its a nice win for site builders.

Cottser’s picture

FWIW, turn twig_debug on in settings.php and you will see the template suggestions for each block instance in HTML comments :)

Jeff Burnz’s picture

#138, yes thats a good tip in general.

I'll clarify #137 for reviewers that don't fully understand what I mean, this is the sort of thing I'm alluding to for the change notice.

The root template is branding.html.twig, however there is no such thing as branding--2.html.twig. For example if you have two instances of the branding block it might be somewhat intuitive for a themer to think that there would be a branding--2.html.twig type template suggestion for instances - there is not.

Additionally there is no way (as it stands) to provide this suggestion via hook_theme_suggestions_branding_alter(), instead the way to theme is to ignore branding.twig.html and use the per block instance suggestion, aka:

  • block--sitebranding.html.twig
  • block--sitebranding-2.html.twig

Here you will print variables from the block $content array, for example lets print the logo in block--sitebranding-2.html.twig, because we want to remove a wrapper or whatever:

<?php
<div{{ attributes }}>
  <
a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo">
    <
img src="{{ content['#site_logo']['url'] }}" alt="{{ 'Home'|t }}" />
  </
a>
</
div>
?>

While this is intuitive and easily grasped by those of us who work with drupal 8 all the time, for new Drupal themers this might be a bit of a wtf moment so having some docs lying around that state it strait out could be useful.

Cottser’s picture

@Jeff Burnz - Great point, I missed what you were getting at with overriding branding vs overriding the block template.

In that case we may want to pass an array of theme hooks to the render array, assuming we have more information about the block instance in the build method of the block plugin.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
@@ -0,0 +1,135 @@
+    $build['#theme'] = 'branding';

Could be $build['#theme'] = array('branding__' . $some_kind_of_id, 'branding');

That doesn't necessarily need to happen here, could be a non-major follow-up issue.

webchick’s picture

Awesome!! I am so happy to see work being done on this! :)

ATM it doesn't seem like there's a change record required for this, since this just adds an optional block... but I was a bit thrown by Jeff's comment in #139... but I think this is true for all block instances, no?

Things that I noticed while testing:

  1. The block looks funny when actually used (left is built-in site branding vs. right is the new block):

    Block is smaller, harder to read, aligned to the right

    ...but I guess fixing this has been split off to #2005546: Modify regions to support Convert site elements (site name, slogan, site logo) into blocks instead. I'm really curious how we plan to solve that, since with these branding elements being blocks, they can now go into literally any theme region.

  2. One thing we are missing is some links here to the proper admin pages to set these things, at least for people with access to them:

    Checkboxes for site slogan, etc. reference names of admin pages without links.

    We leave people up a creek otherwise since they're not going to know where "site information" settings are necessarily (they set them on install, and typically don't have a reason to go back and change them after).

  3. Ideally, you wouldn't need to jump around at all, and could just set those settings right here in-line. I'm guessing that wasn't done here once again to minimize the impact of this patch on the code base. (And needs some thought, since site name has implications elsewhere than just this one block.)
  4. This interface also gets into some "fun" UX problems. For example, if "Site logo" here is checked, but "Logo" visibility is toggled off in admin/appearance/settings, no logo will appear in the block despite the value of this checkbox being true. And worse, if you only have "administer blocks" perms but not also "administer themes" perms (which seems like a reasonable set-up for many site configurations) the block will just appear to be completely broken for you. I would've expected these checkboxes to be informed based on those global settings (or, once again, remove them in favour of this instead).

Given it took us 3 years to get to this point, I'm a bit torn. On the one hand, I'd love to commit this to get a stake in the ground and make some progress on this effort. On the other, though, I'm pretty worried that if we don't take on a pretty hefty chunk of work, which includes at least:

- Make sure this block looks decent and can actually replace the built-in site branding in Bartik.
- Remove the old global theme settings and use the block configuration instead.
- Probably? move the site name/slogan/etc. fields to the block itself vs. on some random disconnected admin page.

...then we leave 8.x in a bit of an inconsistent state with this block that creates more confusion than it helps solve.

So I lean towards committing it, with a "revisit before release" tag so if that follow-up work doesn't get done, we could always revert this patch (that's a big advantage of the "split off" approach that was chosen here). But before doing that, I'd love to hear from the drivers of this patch on what they view as the next steps to get this "shippable" and how much work that looks like.

mdrummond’s picture

I'm not clear if the issue raised in #139 is specific to this patch, although given Cottser's suggested fix in #140, I think that's possible. Cottser seems to indicate this could be addressed in a follow-up issue, but if you think it needs to be handled here, we can do so.

Here are my thoughts on the other issues you raised:

1) The approach I was taking in the branding region follow-up was to ensure that the branding block looked the same when in the same location as it previously was (the newly minted header_first region). I'm not sure that we can or should ensure that the branding block looks the same in every other region. For example, a branding block in a sidebar probably may or may not have enough room to have a longish site name and slogan floated to the right. We could set up some component-based styles that would work in any region, but we'd have to define what styles are important enough for that. Those component-based styles could be defined here or in the branding region issue.

2) Making a link to the site information page, where the site name and site slogan is defined, is easy. The logo, however, can be defined either on the appearance page, or on the theme settings page, which overrides the appearance page setting. A link to the appearance page is simple. A link to the theme settings page gets more complicated.

A block can be used in any enabled theme. So this block could be administered from the block layout page for any theme. Thus a link to the theme settings page would need to know which theme tab on the block layout page somebody was on before they came to this block configuration page. That seems... complicated.

Because of that, we just gave general directions on where to find the settings where these variables are defined.

3) The site name, slogan and logo are global settings that can be used throughout the site. There could in theory be 12 branding blocks on the site. I think having each one define the site name, slogan and logo could get very, very confusing.

4) The plan (at least my plan) is to remove the show/hide toggles for the site name, logo and slogan from the appearance and theme settings page. We were planning to do that in the follow-up issue, since that is where the branding elements will be removed from the page templates for system/Stark and Bartik. At that point those elements will only appear through the block, so those toggles won't make any sense anymore and should be removed. The branding blocks created by this patch actually will not be controlled by those show/hide toggles (at least I think so, based on how this is set up).

My plan on what we'll tackle in the follow-up issue is:

1) Split header region in Bartik (and system/Stark?) into header_first and header_second

2) Install puts a branding block with default settings into header_first

3) Ensure that branding block in header_first region has same CSS as previous branding elements

4) Consider what CSS for branding elements should be component-based, so that it can follow the branding elements to any region.

5) Remove site branding elements from page template in Bartik and system/Stark

6) Change appearance page and theme settings page to remove ability to turn site branding elements on and off.

7) Clean up a bit of the HTML/CSS in the branding block. The name-and-slogan wrapper is kind of weird if there is only a name or only a slogan. Probably there to float the name and/or slogan to the right of the logo. Look if there's another way to do that.

8) Add the ability to target specific instances of a block, as mentioned in #139/140.

Items 1, 2, 3 and 5 are already done in the follow-up patch, at least for Bartik. A little more work needs to be done to get those changes made to system's page template too.

In short, I think it's very doable that we can tackle those problems.

I don't agree that the site name, logo and slogan should be set in the block. That seems like a completely different issue if we wanted to consider it at all.

As for the links to the site information and appearance page. That's easy. It's the theme setting page that's a doozy, and I don't have a good answer for that, which is why I left it off. Personally I don't think that's a deal breaker. If somebody knows enough to be able to manage a block, they probably know enough to navigate to a theme settings page on their own.

Kid's crying, so I gotta go. Interested in your thoughts on the above.

webchick’s picture

Awesome, thanks for the detailed response!

That's a really good point that there might be multiples of this branding block, and the global settings would apply to all of them (I also basically talked myself out of this when I thought about various instances where "Site name" is used: e-mails, etc.). That's a good case for leaving things where they are and just linking to the appropriate admin pages. So forget that terrible idea. :)

Also a good point that linking to the theme is a bit tricky, although it shouldn't be *too* hard. You got to the that settings form in the first place because you clicked "Place block" (or "Configure block") from the block layout page, which indicates which theme it's placing blocks on, so presumably the theme name is somewhere accessible from here.

Thanks a lot for the detailed next steps. Out of those, the ones that feel like they'd be nice to solve in this issue prior to commit are:

- Linking to the correct admin pages from the block config form (I realize this seems pedantic, but without it I worry we're introducing a UX regression if it never gets cleaned up)
- Cleaning up the "name-and-slogan" part of the template
- Adding a means so that the block is targetable by themers (unless this is an issue for all blocks, not just this one?)

The reason I say this is because this block is the first out of X issues that will add a block to replace hard-coded theme variables. As such, it'd be nice if it established good patterns we could then replicate on all of the other patches and make review of those lickety-split. It also cuts down on the number of follow-ups to track, since there are already at least 5-6 of them you've identified.

Not marking down from RTBC though, in case there's push back on that proposal.

mdrummond’s picture

So from the block layout page, if you click to add a site branding block, the path is:

admin/structure/block/add/system_branding_block/bartik

I would imagine we might be able to test the URL args to nab that last bit to get the referring theme and thus a link to the correct theme settings page.

You can create a branding block using the above URL without the theme name at the end. So if the theme name isn't in the URL, maybe drop the link to the theme settings page, particularly because you can then choose to put that block into different regions for different themes.

Does that plan sound ok?

mdrummond’s picture

I'm looking at the name-and-slogan part of this, and this is a bit tricky in terms of the appearance plays out in Bartik.

In Bartik, the logo is floated to the left, and then site-and-name is also floated to the left. That allows the slogan to appear under the name no matter the size of the logo.

If we removed the wrapper div, it gets a bit ugly. You can't just float the site name and the the site slogan to the left, because then the slogan will likely appear to the side of the name.

If the logo were always a certain size, then you could not float the logo, put the name and slogan ahead of the logo, float them to the left and apply a margin-left the same size as the logo. But the logo size will vary widely, and with a very thin logo, you could still get the slogan appearing below the logo.

So... I'd suggest that what we want to do is not remove the wrapper around the name and slogan but instead change its class name. I would suggest site-branding-text as a possible class name.

In theory we could drop the wrapper div from Stark, although if we did I'd suggest putting a wrapper div or p around the site-name's strong element: otherwise the name will appear to the right of the logo, while the slogan appears below the two of them.

I think changing the class name for the name and slogan wrapper to site-branding-text or something similar to that is probably the neatest solution.

Thoughts?

mdrummond’s picture

One other issue:

For the links to the site information page, appearance page and theme settings page, do we need to check access permissions before displaying the link?

I'm not sure if there's a proper D8 way to do that or to check the arg value that might or might not contain the theme machine name. Hopefully I can catch somebody in IRC to walk me through that.

mdrummond’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new20.91 KB
FAILED: [[SimpleTest]]: [MySQL] 64,560 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
new14.77 KB

Ufda.

So, I've done a number of things in this patch.

1) Removed the name-and-slogan wrapper in system/Stark. I also changed strong class=site-name to div class=site-name. This is a visual change, but it allows the site name to stand on its own rather than appearing to the right of the logo. As long as we're making changes, I wouldn't mind if the divs for site-name and site-slogan were p elements instead, but we can leave that for a Dream Markup issue so we don't get into too many quibbles.

2) Added branding.html.twig to Bartik, so that Bartik can have an equivalent of name-and-slogan, which I titled site-branding-text. This allows the name and slogan to float to the right of the logo, but doesn't have such an odd name when only the name or slogan is present.

3) Updated the CSS in Bartik to account for these markup changes. Please note that since we haven't yet removed the markup in Bartik's page.html.twig, the old id-based selectors still need to stay in. Those will get pulled out later. Also note that while this makes the appearance of branding blocks roughly similar in other regions it may not be identical, because some regions change the font size or do goofy things like put a white border around the region (I'm looking at you, header region). I don't think it's our responsibility to override those regional settings for the sake of the branding block. That would be a nightmare.

4) Added the requested links to the configuration pages. That meant doing dependency injection to get an url injector as well as the request module.Yay Symfony! I also had to alter BlockFormController (per Tim Plunkett's suggestion) in order to make the block aware of its own theme in case the theme isn't available in the request information. This also checks to see if the appropriate permissions are available for the appearance/theme settings pages and the site information page. If the permissions aren't available, the block form even provides a helpful little message explaining that.

The one thing this patch doesn't do is allow block instances to get their own twig file. To be fair, however, I can't find a single block in core that does allow for this. I checked with larowlan, and he explained that a block plugin can't know about a block entity for caching reasons. He suggested hook_block_view_alter as an alternative. Or possibly hook_block_view_BASE_BLOCK_ID_alter. I'm not clear on exactly how to bring in a block instance with that, maybe somebody else has some ideas.

So anyhow, that's my evening's work. Seemed to work as I was trying things out. Time for everybody's favorite singalong: "What Does Testbot Say?" (#2203045: What Does Testbot Say?).

Good night and good luck.

tim.plunkett’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -93,6 +93,16 @@ public static function create(ContainerInterface $container) {
    +    if ($entity->get('theme')) {
    +      $theme =  $entity->get('theme');
    +    }
    +    else {
    +      $theme = $this->configFactory->get('system.theme')->get('default');
    +    }

    I would rewrite this as

    if (!$theme = $entity->get('theme')) {
      $theme = $this->configFactory->get('system.theme')->get('default');
    }
  2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -93,6 +93,16 @@ public static function create(ContainerInterface $container) {
    +    $form_state['block_theme'] = $theme;

    @@ -254,6 +263,8 @@ public function form(array $form, array &$form_state) {
    +    $form_state['block_theme'] = $theme;

    Why twice?

  3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
    @@ -0,0 +1,114 @@
    +  public static function getInfo() {
    ...
    +  function setUp() {

    These should have @inheritdoc, and setUp should be protected

  4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
    @@ -0,0 +1,114 @@
    +  function testSystemBrandingSettings() {

    should be public

  5. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,210 @@
    +  public function url($route_name, $route_parameters = array(), $options = array()) {
    +    return $this->urlGenerator->generateFromRoute($route_name, $route_parameters, $options);
    +  }

    This should definitely not be public. Also, do we even need the helper function? Might as well just use $this->urlGenerator in each place.

  6. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,210 @@
    +    // Get the theme.
    +    if ($this->request->attributes->get('theme')) {
    +      // Get the theme from request if available.
    +      $theme = $this->request->attributes->get('theme');
    +    }
    +    else {
    +      $theme = $form_state['block_theme'];
    +    }

    I think you should skip checking the request, and just rely on the form_state. Then you can ditch injecting it completely.

  7. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,210 @@
    +    $administer_themes_access = \Drupal::currentUser()->hasPermission('administer themes');
    +    $administer_site_configuration_access = \Drupal::currentUser()->hasPermission('administer site configuration');

    The user should be injected.

  8. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,210 @@
    +    } else {
    ...
    +    } else {

    Two lines

Status:Needs review» Needs work

The last submitted patch, 147: site-branding-block-1053648-147.patch, failed testing.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new20.4 KB
PASSED: [[SimpleTest]]: [MySQL] 64,621 pass(es).
[ View ]
new8.77 KB

Went through all the changes Tim suggested in #148 and made them all.

We also spent a good amount of time working through how one might be able to target the instance of a branding twig file.

The short version is that this might not be possible at present. If we were dealing with a block twig file, there's already a block suggestions hook to take care of that. But since this isn't actually a block, it's the content inside of a block, it's too deep to be able to get the entity_id for the suggestion basically.

I'm guessing the alternatives are to maybe rewrite the twig file so that it's a block in all its glory or maybe to create a new suggestions hook to handle twig files like this. It seems a bit heavy to put all the regular block code inside of the twig file, so I'm guessing this may be a larger issue that needs to be solved elsewhere.

Hoping this goes green, all the problems are magically solved, and we can give the thumbs up. I'm on the v12 branch for this patch, but who's counting?

mdrummond’s picture

Too late for me to check this tonight but block.module declares it's own suggestions for block templates, so maybe system.module needs to do the same for branding, if that's even possible?

Jeff Burnz’s picture

I should clarify #139, I'm not really saying we should have suggestions. What I actually meant is, if possible, to provide a way for themers to add suggestions - if they wanted to. Right now this is totally themable as it is, #139 is/was a feature request, I wouldn''t let this hold up the issue at all. I'll review this later today also, right now punished for time.

mdrummond’s picture

Right now, system.module has the following suggestions:

  • system_theme_suggestions_html
  • system_theme_suggestions_page
  • system_theme_suggestions_maintenance_page
  • system_theme_suggestions_region

That covers some of the templates for system, but not the other 18, including branding.

The question would be what information would be available to system_theme_suggestions_branding in $variables['elements'].

block_theme_suggestions_block uses:

  • $variables['elements']['#configuration']['module']
  • $variables['elements']['#plugin_id']
  • $variables['elements']['#block']->id()

If similar information was available to us, I would imagine we could set up a suggestion hook similar to block's allowing robust theming of this. I have to run to work, so I can't test this right now, but if anybody has thought on where some of that info would be available in a branding hook, that would be helpful. When I was doing testing last night, it was the entity ID that had the right hook for a branding instance.

I do think that it's a good goal to make instances of this themable. I think it's much, much easier to explain to themers that a theme's twig files can override core's twig files or an instance of a twig file, rather than saying some can have instances and others can't. Maybe Twig debug provides that info as to what suggestions are available?

Anyhow, patch above went green, so hopefully we're getting really, really close.

tim.plunkett’s picture

I don't quite understand the need for a separate branding.html.twig template.
I think that custom block--system_branding_block.html.twig templates can be used, there isn't *that* much extra wrapping it.

mdrummond’s picture

StatusFileSize
new21.12 KB
FAILED: [[SimpleTest]]: [MySQL] 64,748 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new7.49 KB

If you thought, hey, the 13th branch for this patch is likely going to be the lucky one, let me inform you: you are wrong.

This patch does not work, but hopefully it's a step in the right direction, in that with a few pointers, this can work, or we decide this approach is unworkable.

Per a discussion with Cottser and Webchick on IRC, this patch tries to do the following:

1) Converts branding.html.twig from a template for the contents inside of a block to the entire block itself. But wait, there's more!

2) Creates a block within a block to extend the block contents. This Twiggy goodness means that—in theory—the block wrapper HTML can still be controlled from block.html.twig while this template just reworks the content based on the available variables.

3) ???

4) Profit.

The problem is that the block contents don't show up, which seems marginally problematic. The suggestions function in block.module doesn't seem to like the block ID, nor does it like the elements in general.

I'm a bit nervous that #1989568: Remove block config ID from being used as an HTML ID or template suggestion might render the whole issue moot. Without a config id, I'm not sure how you get the block instance. Entirely possible I'm not understanding, though.

You're welcome to tell me the things I am doing wrong in this patch. There are probably a good deal of them.

tim.plunkett’s picture

That linked issue would indeed make this override dead in the water.

But the OP asks a question:

What are use-cases where an HTML ID is needed or where you'd want to implement a template override on a per-instance basis (i.e., you added two blocks with the same exact configuration, but different machine names, and you want their markup to be different)?

This is the first time anyone has come up with a real answer to that.
The other answer is that the "branding block" is doing triple duty, and is overkill.

Status:Needs review» Needs work

The last submitted patch, 155: site-branding-block-1053648-155.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
StatusFileSize
new21.39 KB
FAILED: [[SimpleTest]]: [MySQL] 64,752 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new7.53 KB

@mdrummond - I want to thank you for your continued work on this issue. Please don't get discouraged, issues often change direction and in my experience having 13+ versions of a patch is not necessarily unusual :) we're all collaborating here towards a good solution.

Here's a rough proof of concept patch based on what was discussed in IRC yesterday. This is relying on a pre-existing theme suggestion provided by block.module.

Please ignore the set tags and render_var stuff in the Twig templates for now. That is just an ugly workaround for #953034: Themes improperly check renderable arrays when determining visibility.


The most drastic change from the previous patch is that what you configure in the block settings determines what variables are available in the template. With this render array + block template approach, checking all the block toggles in the template would be really ugly, see #2160611: Provide {{ item.item.alt }} Twig syntax for getting data from $item['#item']['alt']. Here's a sampling of what it would look like without the previously-mentioned issue:

{% if content['#use_site_name'] or (content['#use_site_slogan'] and slogan) %}

If there's a use case for having the template disobey the block settings, that's something that would still be possible to achieve via preprocess. The visibility toggles are something I really don't think needs to be available in this template out of the box but maybe I'm missing something.

Status:Needs review» Needs work

The last submitted patch, 158: 1053648-158.patch, failed testing.

Cottser’s picture

I think the fails in the last 2 patches are whitespace related, by the way. So nothing to worry about at least while we are trying to decide the best way forward.

mdrummond’s picture

I don't understand why we need to switch to render arrays rather than providing the variables directly to the template as we did previously.

What we had talked about yesterday was turning the branding template into a block template that contained the branding variables, then using extends to pull in the wrapper parts of the template.

That's what I tried to do in #155, although admittedly it wasn't working quite right. We should still be able to provide the variables directly to the the block--system-branding.html.twig template, right? And can't the Bartik template just override that just like any other Twig template?

Cottser’s picture

I don't think that's possible. If you look at block_theme() it doesn't define 'variables' so we can't add to that when we define the block__system_branding_block theme hook. The block theme hook instead defines a 'render element'. Trying to switch to 'variables' for this specific instance will break things badly because preprocess functions, theme suggestions functions, etc. are based on 'render element', not 'variables'.

Edited x2 to clarify wording.

mdrummond’s picture

I'm going to ask a few questions that are probably dumb.

1) Is there a way for system.module to provide a hook extension of template_preprocess_block to add additional variables for the branding block?

2) It seems like the main thing we're getting from making this a full-fledged block is the suggestions function already in block.module. Couldn't we do a system_theme_suggestions_branding in system.module to create instance naming suggestions? I'm guessing the answer is that we wouldn't have access to the plugin_id or the config id? Seems like something that might be worth playing around with.

tim.plunkett’s picture

#161, this is just the difference between block_theme() using 'render element' instead of 'variables'. You can still put anything on the $variables['element'] you need.
#163.1, we want to avoid system module having block code.
#163.2, same answer. block_theme_suggestions_block() is dependent on code in template_preprocess_block(). A separate branding template cannot use any of that information, because it is two levels away: branding -> block plugin -> block entity

mdrummond’s picture

Now that I'm understanding this better, it makes a lot more sense.

I'll have to take a look at the visibility discussion above to see if there was a use case for providing the variable to templates even if the use is turned off. Using #access to determine visibility is pretty clever.

Would be nice to see #953034: Themes improperly check renderable arrays when determining visibility get in, but if it's not ready before this issue is set to go, we can create a follow-up issue to address that.

As per earlier discussions, we probably want the variables to be $site_logo, $site_slogan and $site_name.

Just so I'm clear, are we essentially putting the variables in via the build step rather than via system_theme?

I think we can probably get this ready and then have a follow-up for #953034: Themes improperly check renderable arrays when determining visibility.

dcrocks’s picture

This patch has doubled in size since #135. I'm not sure what the extra gets me. I know with #135 I can have a theme local copy of branding.html.twig where I can add more themeing handles if I want and modify the markup as well. True, there aren't many useful vars available but I can do some conditional stuff(e.g., I can change the output depending on whether I'm logged in or not). I thought the issue here was to implement the new block and have a separate issue to convert core themes to the new block. Am I really going to do very complex things with this block?

mdrummond’s picture

dcrocks:

By making the Twig template a full-fledged block, that allows for it to receive block suggestions for the template name. That means that in your theme, you can still have a local copy of this template to alter as you please. However, if you want to put a branding block in three different regions, and you want to adjust the markup a bit in one or two of them, you will be able to create a separate branding block template file to target a particular block instance.

That might not happen all that often, but this patch is setting a pattern for a number of other issues that are similar, for example menu blocks. You may want a different markup pattern for a menu block with drop-downs that would appear in the header, while you'd have a different markup pattern for a vertical menu that appears in your sidebar.

I think that's a good pattern to establish and is worth a little extra effort to get there.

You can still do all the customization you want with pretty much the same variables. If you want to make the site slogan conditional upon a user's role, you can do so.

The follow-up issue to this one will replace the default branding markup in Bartik and system/stark with a header region where a branding block can be placed on install.

Jeff Burnz’s picture

FWIW I'm pretty much agnostic with regards to the approach taken here, I like both to be frank (pros/cons on both sides from my pov).

My question is how does this affect the other issues going forward - e.g. in breadcrumbs, status messages and the like? Right now those already have separate templates and was one reason I kind of like #135, it seemed more consistent with those approaches (thinking that status messages template would work like the branding.html.twig does in #135).

Like I said I'm not, at the moment, swayed either way, whatever is most consistent and easiest to understand is good (win for themers IMO), to be frank I found the template suggestion naming conventions imposed by #158 frightfully confusing.

That said, if there are compelling reasons (that I am blissfully unaware of) for doing #158 and we make this consistent for core, then right oh then, do it.

I hit one, possible unrelated but pertinent issue here - when I {{ dump() }} in the block you get a twig error:

Twig_Error_Syntax: A template that extends another one cannot have a body in "core/themes/stark/block--sitebranding-2.html.twig" at line 21. in Twig_Parser->filterBodyNodes() (line 370 of core/vendor/twig/twig/lib/Twig/Parser.php).

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new21.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here's a revised patch that uses site_logo instead of logo and site_slogan instead of slogan.

The only markup change in #158 was generating the img element within the build step rather than in the twig file. Is there a reason we need to do that rather than just passing the URL to the template and constructing the img element there? That makes it easier to add a class to the img if necessary.

Also, is there a reason the Twig file is named block--system-branding-block vs block--system-branding? I'm guessing we can't use block--branding either?

mdrummond’s picture

I read through #953034: Themes improperly check renderable arrays when determining visibility and geesh, the issue goes back 3.5 years with the Twig part at least 9 months. There's talk of a solution, but nothing concrete in terms of a patch that's close to being committed. So who knows how long it will take to fix that.

Setting and rendering the variables in the Twig template just to be able to properly test #access seems gross to me and probably will to other themers too, as Jeff indicates in #168. I'm also not thrilled with something like content['#use_site_name'].

Tim indicated in #164 that we can somehow still put things into $variables['element']. Any thoughts on how we do that? That would allow us to make use_site_name available in the template again without having to resort to content['#site_name'].

As for #168, I have no idea what the error about body in an extended template means. Strange.

In terms of how this would affect the other similar issues. If we take this approach for the others it means making full block templates with extends for each of those variable to template conversions. That's probably particularly important for the menu issue, as that seems to me the likeliest place that somebody will want to adjust markup in different block instances.

Hopefully we can get use_site_name back into the template and address the possible error in #168. Other outstanding issues are the block template name options and whether or not we can just pass the logo URL to the template and construct the img element there.

mdrummond’s picture

StatusFileSize
new21.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,750 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.87 KB

Error in the filenames.

The last submitted patch, 169: interdiff-1053648-158-168.patch, failed testing.

Cottser’s picture

To get it out of the way, the twig error brought up in #168 looks like it was due to putting dump() inside the "body" of the template extending block.html.twig. You can see the same error by putting {{ 'foo'|t }} outside of the {% block content %}…{% endblock %}. Because this is a template extending another template, the places you work in are the block tags, to optionally override the blocks defined in the parent template.


The template naming of block--system-branding-block is based entirely on existing block suggestions. That one in particular is based on the block's plugin ID set in the annotation:

* @Block(
*   id = "system_branding_block",
*   admin_label = @Translation("Site branding")
* )

I'm not sure what the naming conventions are for block IDs but to change that particular theme suggestion we do have the option of changing the plugin id. I'm guessing we'd want the system namespace at least, so maybe 'system_branding', but it seems like most other block plugins end in _block. The benefit of using block theme suggestions is that whatever core or contrib theme suggestions there are for a given branding block instance, they can be used. I used this suggestion because it should always be there unless someone forcibly removes it. If a module or theme hook_theme_suggestions_HOOK_alter()'d out this theme suggestion the branding elements will still render through the default block.html.twig because the block contents are just a render array. So the custom template is not really needed because we've built this out as a proper render array.


Regarding #953034: Themes improperly check renderable arrays when determining visibility we can always render the three parts of the render array via a preprocess function and drupal_render(), unfortunately that limits the amount of further manipulation possible but it would be only temporary because #953034: Themes improperly check renderable arrays when determining visibility needs to be solved before release (it's a critical issue).

Setting and rendering the variables in the Twig template just to be able to properly test #access seems gross to me and probably will to other themers too, as Jeff indicates in #168. I'm also not thrilled with something like content['#use_site_name'].

Tim indicated in #164 that we can somehow still put things into $variables['element']. Any thoughts on how we do that? That would allow us to make use_site_name available in the template again without having to resort to content['#site_name'].

I just want to note that we'd never ship a template in core with the set blocks and render_var stuff. It's only there as a proof of concept.

It looks like the block plugin's build method adds to $variables['elements']['content'] and I'm not sure we'd want to try and rewire that for our use case. The reason why we can't arbitrarily add variables to this render array is because it's a render array, so there are many points in the process where the array will go through things like element_children() and if you have $variables['elements']['content']['use_site_name'] = 1 you'll see an error like:

User error: "use_site_name" is an invalid render array key in Drupal\Core\Render\Element::children() (line 89 of core/lib/Drupal/Core/Render/Element.php).

Arbitrary data like this needs to be stored in a hash-prefixed #key in render arrays otherwise it's treated as a render element. The hash-prefixed keys are ugly in Twig templates when there are use cases for pulling out that arbitrary information and that's why I opened #2160611: Provide {{ item.item.alt }} Twig syntax for getting data from $item['#item']['alt'] a while back.

So if we want to push forward with this general direction of using a block suggestion, we should decide whether we want to use the #access and temporarily drupal_render() method, or whether we want to store the #use_site_name, #use_site_slogan, #use_site_logo information in the render array and pull that information out in the Twig template, even if the syntax is ugly at this point. Or maybe there's another way we haven't thought of yet.

The last submitted patch, 169: site-branding-block-1053648-168.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 171: site-branding-block-1053648-171.patch, failed testing.

Cottser’s picture

I also wanted to mention that I've added this issue as an agenda item for the weekly Twig/theme system call that's happening in about 1 day + 7.5 hours at the time of writing. Anyone following this issue is welcome to join in and we can discuss how to move forward in real-time instead of composing novels to each other :)

http://everytimezone.com/#2014-02-27,720,6bj

If you join #drupal-twig on IRC we'll post the link there when the hangout starts, or you can keep an eye on @Cottser on Twitter, I'll tweet out the link to the hangout from there as well.

mdrummond’s picture

1) Keeping name as block--system-branding-block seems fine. If that works better for the suggestions system, cool.

2) If we can use a preprocess function for now to handle the drupal_render and access checks, maybe that would be a good way to go. If we're going to have a temporary solution until that issue is fixed, that at least allows us to keep the markup in the Twig file the same. Although now that I think about it, once we don't need the preprocess, we'd probably ultimately still need to change the markup, as then we'd be bring content['#use_site_name'] or ideally content.use_site_name back into the Twig file, right?

So I guess either way would require a change. Maybe it's better to put the set blocks and render_var into the template for now. If somebody pulls that into a theme now, it will still work after it's no longer necessary. However if we go the preprocessor route, then pull the preprocessing, then if somebody doesn't update their template there will no longer be an access check.

3) Only other question I have is if we can push the logo URL into the variable rather than the entire img, so that there's more flexibility with the img element markup in the template.

---

I may or may not be able to make the Twig call: I was only able to make last week's because I was working from home. If I make the call, it will likely be at the end. Unless I can figure out how to do the Hangout audio-only on my phone while I'm in the car during my commute. If this item could be at the end of the agenda, that would be great.

joelpittet’s picture

tl;dr
Re: #177 I'm going to push back too on the renderable arrays being passed in to the template. The #access trick is nice but the theming experience (FX as #mortendk calls it) has become a pain in the ass at that point. Would rather have the "if conditions" in the template then have to modify generated markup _alter hooks to modify an array to spit out an HTML tag with an extra class.

Cottser’s picture

To address the last couple comments:

If we use the preprocess function to flatten out the render arrays – that is a hypothetical approach but *should* work – I don't think we'd change things later to use content['#use_site_name']. However, depending on the solution decided on in #953034: Themes improperly check renderable arrays when determining visibility we may need to modify the template slightly to call out to a special function/test.

If the render array is just spitting out:

  1. A URL to a logo image
  2. Text for the site name
  3. Text for the site slogan

If you want the URL or either bits of text in your template, you check the checkbox in the block config. Done. Any markup/classes you want to put around those items is completely up to you in your Twig template(s).

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new22.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,713 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.25 KB

I think we have our winner. We now have the same nice neat templates as in #135, but we also have all the benefits of having a block template. The magic comes in the preprocessing of our variables. We're providing the atomized logo URI to the template along with all the use_site_logo type variables. Even if somebody removes the suggestion that pulls in these templates, the logo, site name and slogan can still show up through just the render array. So I really think we have the best of both worlds.

Hopefully this passes, we can get some eyes on it, make any last fixes, and ship this!

He says hopefully.

Status:Needs review» Needs work

The last submitted patch, 180: site-branding-block-1053648-180.patch, failed testing.

mdrummond’s picture

mdrummond’s picture

Status:Needs work» Needs review

"Previous result: FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: @reason."

That's a new one. Setting to re-test.

Status:Needs review» Needs work

The last submitted patch, 180: site-branding-block-1053648-180.patch, failed testing.

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new23.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,717 pass(es).
[ View ]
new1.56 KB

Fixed some whitespace issues in the block test, now that we added a block to our block so we can extend while we extend. Thanks to timplunkett for finding that.

mdrummond’s picture

We have a green patch that does all the magical things we want it to do. Feedback and manual testing welcome!

dcrocks’s picture

StatusFileSize
new63.36 KB
new66.43 KB

Tested #185. Installed successfully on a new clone of D8. Added new block to 2 regions OK, one with just logo and one with title and slogan. Still think this patch should not contain anything bartik specific in it. That should be part of follow on, otherwise time wasted on getting styling correct instead of getting this patch in. See 1st image. Created 2 override templates, block--sitebranding.html.twig and block--sitebranding_2.html.twig. Output shown in image 2.

mdrummond’s picture

By having a Bartik template and providing CSS adjustments, that allows the branding block to look relatively similar to the one built in to the template. The only discrepancies right now should be from different default font sizes for different regions, and the goofy white border built into the region CSS. I don't think we should do anything about those, as those are separate issues and not necessarily errors.

If we don't put in the Bartik template and CSS adjustments, the branding elements will look much more broken in Bartik.

This isn't replacing the logo, site name and slogan in Bartik's header, so I don't think things have to look precisely the same. Things should look decent, but they don't have to look perfect.

It looks like the logo is maybe getting cut off in those images? It's set to float to the left, so I'm guessing maybe we need a clearfix attached to it?

mdrummond’s picture

Accidentally pressed save more than once. Sorry.

mdrummond’s picture

Another accidental save. Sorry.

dcrocks’s picture

There is no change to the current appearance of bartik when this patch goes in so why the extra code.The followup that converts the core themes to use these blocks should do all the theming as well as removing the old implementation. Until that goes in this patch is invisible to bartik or any other theme and the code you added is really only useful(but not necessary) for testing.

mdrummond’s picture

This patch has taken three years to get to this point. I'd like to think that if this goes in, the patch to replace the branding variables in the template with a branding block will go nice and smooth, but there's no guarantee of that.

In the meantime, this patch will allow branding blocks to be placed into regions within Bartik: they aren't invisible. All we do in this patch is to make sure that those branding blocks look relatively similar to the existing branding elements. If we don't do that, the logo, name and slogan will look pretty broken when they're placed into a region.

dcrocks’s picture

Status:Needs review» Reviewed & tested by the community

This is not worth arguing about. The issue is almost 200 comments long, the patch has had review, and it has been manually tested. Let's get it in.

Jeff Burnz’s picture

Status:Reviewed & tested by the community» Needs work

#185 patch prints the empty site slogan wrapper, if the slogan is empty but checked in the block config you get empty markup, ideally this should not happen, i.e.:

$variables['site_slogan'] = $variables['content']['site_slogan']; does not tell us if $variables['content']['site_slogan']['#markup'] is not an empty string. We need some checks.

Personally if using preprocess I will push it all the way and use it to full effect, for example:

<?php
$variables
['site_slogan'] = '';
if (
$variables['content']['site_slogan']['#access'] == 1 && !empty($variables['content']['site_slogan']['#markup'])) {
 
$variables['site_slogan'] = $variables['content']['site_slogan'];
}
?>

And simplify the template even more. $variables['use_site_slogan'] can still be set in preprocess and available to themers etc. Not sure about how you guys or Drupal core views such approaches (pushing more logic back into preprocess). AFAICT there is/has been some effort to bring logic into templates - at least the simple stuff, in any regard the empty wrapper should not print and we need to check for #markup being empty or not.


Use of details element - this is rather inconsistent with other core blocks such as Recent Comments, Recent Content etc. Are we sure we want to hide the checkboxes in this fashion (a sort of progressive disclosure) or should we just show them? If the goal is to just group items (accessibility) then possibly fieldset is more appropriate (which would also be inconsistent with other core blocks, they do not use any wrapper).


Sorry I could not make the hangout, I am painting and decorating and getting hounded to finish it soon :)

mdrummond’s picture

StatusFileSize
new23.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,718 pass(es).
[ View ]
new7.9 KB

Here's a patch that brings the access check into preprocess, allowing us to pull the use_ variables from the template. Also changed the block form from details to fieldset.

In Bartik, the logo is going to be cut off anywhere without a clearfix class. This is not easy to solve without putting a stupid extraneous div in the template, because we don't know the class names of any of the parent divs.

Ugh.

mdrummond’s picture

Status:Needs work» Needs review

Come at me, Testbot!

Cottser’s picture

Status:Needs review» Needs work

This is looking good and looking very close to me! Here's some minor points that can be fixed up:

  1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -93,6 +93,13 @@ public static function create(ContainerInterface $container) {
    +    // Store theme settings in $form_state and use them in the theme settings below.

    'below' makes this over 80 characters, please wrap it onto the next line (or reword ;)).

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,202 @@
    +      // Provide links to the Appearance and Theme Settings pages
    +      // if user has access to administer themes.
    ...
    +      // Explain that user does not have access
    +      // to the Appearance and Theme Settings pages.

    I know it can be tempting to make these pretty and not have widows and stuff but the documentation standards state that these have to be as long as possible within 80 characters so these should be re-wrapped. https://drupal.org/node/1354#drupal

  3. +++ b/core/modules/system/system.module
    @@ -152,6 +152,10 @@ function system_help($path, $arg) {
    +    'block__system_branding_block' => array(
    +      'base hook' => 'block',
    +      'template' => 'block--system-branding-block',
    +    ),

    I think it might be worth adding a comment explaining this since I think this method is only used in one or two other places in core. Something along the lines of:

    "Normally theme suggestion templates are only picked up when they are in themes. We explicitly define the block__system_branding_block theme suggestion here so that the template in core/modules/system/templates is picked up."

  4. +++ b/core/modules/system/system.module
    @@ -1416,6 +1420,21 @@ function system_user_timezone(&$form, &$form_state) {
    +      if ($variables['content']['site_logo']['#access'] == 1 && $variables['content']['site_logo']['#uri']) {
    ...
    +      if ($variables['content']['site_name']['#access'] == 1 && $variables['content']['site_name']['#markup']) {
    ...
    +      if ($variables['content']['site_slogan']['#access'] == 1 && $variables['content']['site_slogan']['#markup']) {

    I think we should lose the == 1 from these. #access is boolean anyway so just if is fine here.

mdrummond’s picture

StatusFileSize
new24.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,722 pass(es).
[ View ]
new5 KB

This patch fixes the visual problem in Bartik with the logo being cut off by adding clearfix to the Bartik branding blocks through a preprocess function. Also makes all the fixes Cottser suggested above.

mdrummond’s picture

Status:Needs work» Needs review

Testbot... engage.

mdrummond’s picture

Green! Cottser plans to look this over one more time tomorrow, and if it looks good, RTBC.

Cottser’s picture

StatusFileSize
new28.28 KB

Sorry, a few more minor documentation fix-ups before I'm willing to give this the green light. Leaving at needs review though. I manually tested this and reviewed the patch and can't find any faults.

Once the minor points below are addressed I'm happy to RTBC this. I read through #141-143 again and I think we've addressed the concerns there and then some.

  • We have links to where the logo, site name, and slogan can be changed.
  • We have per-instance theming.
  • The block looks good in Bartik:
  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
    @@ -0,0 +1,202 @@
    +      // Provide links to the Appearance and Theme Settings pages
    +      // if user has access to administer themes.
    ...
    +      // Explain that user does not have access to the Appearance and Theme
    +      // Settings pages.
    ...
    +      // Provide link to Site Information page  if user has access to
    +      // administer site configuration.
    ...
    +      // Explain that user does not have access to the Site Information page.

    I should have read these comments more closely the first time through, I kind of stopped when they were wrapped too soon. I think these would all read better (less terse) if we add the word "the" before "user".

    The second last one of these has two spaces in between "page" and "if" on the first line.

  2. +++ b/core/themes/bartik/bartik.theme
    @@ -121,6 +121,16 @@ function bartik_preprocess_node(&$variables) {
    + * Implements hook_preprocess_HOOK() for system branding block templates.

    This might be expanded to apply other block overrides down the line so let's just say "for block templates" in the docblock please.

mdrummond’s picture

StatusFileSize
new24.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,760 pass(es).
[ View ]
new2.97 KB

This patch fixes the above issues. Maybe this patch will be the one to make it in!

Cottser’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new24.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,758 pass(es).
[ View ]
new1.65 KB

More nitpickery. I'm sure @mdrummond is getting sick of me by now :) so I'm just rolling it in here and RTBC'ing. Super super minor stuff.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Awesome work! This addresses all of my concerns. Love the use of Twig's extend mechanism for this. Thanks so much for sticking with this to find the most optimal solution. Hopefully the rest of these patches will be smooth sailing now. :)

Committed and pushed to 8.x YES! Also? HAPPY BIRTHDAY, MDRUMMOND! :D

Status:Fixed» Closed (fixed)

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