Problem/Motivation

All classes are produced in a preprocess that makes it hard for a themer to manipulate these into whatever they need.

Proposed resolution

Add all classes at the top of the Twig templates, when applicable (more than 3 classes or classes built with logic).

{%
  set classes = [
    'block',
    'block-' ~ provider|clean_class,
  ]
%}
<div{{ attributes.addClass(classes) }}>

Preprocess Functions Modified

core/modules/block/block.module: template_preprocess_block CONVERTED
core/modules/search/search.module: search_preprocess_block LEFT AS CONTRIB WOULD ADD
core/modules/system/system.module: system_preprocess_block LEFT AS CONTRIB WOULD ADD
core/themes/bartik/bartik.theme: bartik_preprocess_block LEFT AS CONTRIB WOULD ADD

Twig Templates Modified

core/modules/system/templates/block.html.twig CONVERTED

core/themes/bartik/templates/block--system-branding-block.html.twig EXTENDS from block.html.twig

Test instructions

Create a new node.

Make a note of the markup that a field produces.

Apply the patch.

Now check the markup of the field again it should match the markup from before the patch was applied.

Files: 
CommentFileSizeAuthor
#24 move_block_classes_out-2328913-24.patch4.26 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,183 pass(es). View

Comments

crowdcg’s picture

Issue summary: View changes
lanchez’s picture

Assigned: Unassigned » lanchez
Issue tags: +FUDK

I'll look into this now.

lanchez’s picture

Status: Active » Needs work
FileSize
3.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,834 pass(es). View

Ok so, here's a patch that removes classes from template_preprocess_block and adds them to block twig template. I also fixed tests that failed.

There's still stuff to do with blocks that wants to add more classes. Pretty much system_preprocess_block and bartik_preprocess_block are only ones. I couldn't really figure out how system_menu_block works so kinda need help with that.

crowdcg’s picture

Thanks @lanchez! Also, need to remove content_attributes, title_attributes, etc. that add classes in preprocess. For instance, in the case of $variables['content_attributes']['class'][] = 'content';. Since this is the only class added and it is simple, you can add it directly inline like:

<div{{ content_attributes.addClass('content') }}>

lanchez’s picture

Yes, I was thinking about that but there's a separate issue for it http://drupal.org/node/1972122

crowdcg’s picture

Ah, hadn't seen that. So this issue will depend on that going in.

Cottser’s picture

I don't think this should be blocked on #1972122: Remove the DIV tag around block content, this is much more straightforward and either one can be rerolled :)

I think we should focus on template_preprocess_block() and block.html.twig in this issue.

system_preprocess_block() and bartik_preprocess_block() are such special cases, if we want to add more templates for those that's fine but not sure it has to happen in this issue.

search_preprocess_block() should be left as is IMO, it's an optional module. If we want to pull that out it and create a template it can be its own issue.

Thanks for working on this @lanchez!

lanchez’s picture

Status: Needs work » Needs review
FileSize
3.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,833 pass(es), 1 fail(s), and 0 exception(s). View
970 bytes

Just moved content class to twig. This might fail tests since class order did change on some blocks.

So should we do more in this issue or not?

Status: Needs review » Needs work

The last submitted patch, 8: moved-content-class-2328913-8.patch, failed testing.

Cottser’s picture

I think we can fix up the tests and wrap this one up. Thanks @lanchez!

lanchez’s picture

Assigned: lanchez » Unassigned
FileSize
4.27 KB
673 bytes

Now the test is "fixed" if you know what I mean. The classes should be checked from rendered block, but I'm not very good with tests. So either it's fine like that or someone should the test better.

lanchez’s picture

Status: Needs work » Needs review
joelpittet’s picture

Assigned: Unassigned » amitaibu

@lanchez Nice work! That fail seems to be on the content class test. Do you think you can try debugging that?

The issue summary suggests we should look at all those preprocesses and templates suggestions. There is a chance we don't need to do anything for those but we should address them never the less in-case we miss something.

joelpittet’s picture

Assigned: amitaibu » Unassigned
FileSize
4.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2328913-11_0.patch. Unable to apply patch. See the log in the details link for more information. View

Wow sorry, I cross posted with you and all sorts of things went weird. Here's a re-upload for testbot.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! I manually tested and things (attribute order and class order) moved around a bit but nothing of consequence.

joelpittet’s picture

+++ b/core/modules/block/src/Tests/BlockPreprocessUnitTest.php
@@ -50,7 +50,7 @@ function testBlockClasses() {
-    $this->assertEqual($variables['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes');
+    $this->assertEqual($variables['content_attributes']['class'], array('test-class'), 'Test-class class added to block content_attributes');

Just an FYI regarding the test. It's correct the way you did that. I didn't think so at first but it's testing what comes of the preprocess and that is not in the preprocess, so to test that you'd have to render the block and do xpath or something. Though the plan is to remove that class anyways so... that'd be total busy work... RTBC +1

joelpittet’s picture

Oh wait, @Cottser thoughts on #13 second paragraph regarding the issue summary?

joelpittet’s picture

Status: Reviewed & tested by the community » Needs work

Actually yeah it needs work,

function search_preprocess_block(&$variables) {
  if ($variables['plugin_id'] == 'search_form_block') {
    $variables['attributes']['role'] = 'search';
    $variables['content_attributes']['class'][] = 'container-inline';
  }
}

Needs to be moved into a search template, and the block classes need to be added there as well.

As well as the other items mentioned in the issue summary.

Cottser’s picture

@joelpittet I addressed that in #7. I think if we want to go that far and add all those templates (don't forget we need to add them to hook_theme() as well if they are new module templates) then IMO that should happen in a separate issue.

Personally I think things like search_preprocess_block() are not defining default classes but rather are acting the way contrib will, just adding classes to the default list. I think we should be focussing more on template_preprocess_* functions, that is where we will have the biggest impact. Because the reality is there are classes coming from other places than preprocess so this initiative as it is will not be removing all classes from core markup.

joelpittet’s picture

@Cottser I get ya on the preprocess, what about system branding block, we've lost classes on those blocks that were previously there. Which I'd expect a visual regression,no?

joelpittet’s picture

Will check for myself after breakfast:p

joelpittet’s picture

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

Ok Updated the issue summary, and back to RTBC. The other blocks are using @extend :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2328913-11.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,183 pass(es). View

Rerolled

mdrummond’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Patch still applies. Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Let's open a followup to get rid of BlockPreprocessUnitTest it looks completely pointless.

Committed 4d2c54d and pushed to 8.0.x. Thanks!

  • alexpott committed 4d2c54d on 8.0.x
    Issue #2328913 by lanchez, lauriii, joelpittet | crowdcg: Move block...
joelpittet’s picture

Status: Fixed » Closed (fixed)

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