(First, verify that the preprocess changes have been made. #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.)

  1. Copy the Twig templates from the core module's templates directory to Classy's templates directory. Include all templates, even ones without classes.
  2. Remove all classes from the core module's template. Remove all classes added with addClass and ones that are hard-coded in the template.
  • If there are classes that are required for basic functionality, discuss whether they should be kept.
  • If there is CSS from the module, or anywhere else, referring to the class, discuss removing it or moving it to Bartik&Seven. Do not move the CSS to Classy.

Twig Templates to Copy

core/modules/block/templates/block-list.html.twig
core/modules/block/templates/block.html.twig
core/modules/block_content/templates/block-content-add-list.html.twig

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

Issue summary: View changes
mortendk’s picture

Assigned: Unassigned » mortendk
Status: Active » Needs review
FileSize
2.35 KB

move the classes outta core and into class

the discussion about which classnames we want in core should be done as a followup issue

Status: Needs review » Needs work

The last submitted patch, 2: classy-block.diff, failed testing.

emma.maria’s picture

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

Assigned: emma.maria » Unassigned
Status: Needs work » Needs review
FileSize
5.44 KB

The last patch had template files straight in the root of classy and not in a templates folder. I have fixed this :)

emma.maria’s picture

Further work...

I have moved block-content-add-list.html.twig into classy/templates/block_content/block-content-add-list.html.twig
I have moved the other templates into classy/templates/block/*

The last submitted patch, 5: block-to-classy-2349625-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: block-to-classy-2349625-6.patch, failed testing.

davidhernandez’s picture

The templates should go into the root of the templates folder, not a subfolder.

mortendk’s picture

Status: Needs work » Needs review
FileSize
2.37 KB

moved the templates to classy/templates

Status: Needs review » Needs work

The last submitted patch, 10: copy_block_and-2349625-10.patch, failed testing.

davidhernandez’s picture

Please double-check if any removed classes are being used in javascript. It is best to test the affected template using Stark to make sure nothing is broken.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: copy_block_and-2349625-10.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review

reroll

DickJohnson’s picture

Assigned: Unassigned » DickJohnson
lauriii’s picture

Assigned: DickJohnson » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
FileSize
8.97 KB

This patch is in wrong issue. Ignore

lauriii’s picture

FileSize
1.49 KB

Right patch finally

Status: Needs review » Needs work

The last submitted patch, 19: copy_block_and-2349625-19.patch, failed testing.

The last submitted patch, 18: copy_block_and-2349625-18.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: copy_block_and-2349625-19.patch, failed testing.

DickJohnson’s picture

Assigned: Unassigned » DickJohnson
DickJohnson’s picture

Status: Needs work » Needs review
FileSize
5.43 KB

Tried to fix the tests.

DickJohnson’s picture

Assigned: DickJohnson » Unassigned

Status: Needs review » Needs work

The last submitted patch, 25: copy_block_and-2349625-25.patch, failed testing.

DickJohnson’s picture

Status: Needs work » Reviewed & tested by the community
DickJohnson’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, wrong issue.

saki007ster’s picture

Assigned: Unassigned » saki007ster
saki007ster’s picture

Removed classes from the template files of block and block_content module.

saki007ster’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 31: copy_block_and_block_content_templates-2349625-31.patch, failed testing.

emma.maria’s picture

@saki007ster Thanks for the patch. Unfortunately your patch does not contain work from the previous patches submitted in the issue. You need to read through the issue and take the most latest patch that needs work and fix the problems reported by others.
So in this case take the patch from #25 apply it to your local copy of Drupal and fix the problems with it, in this case the tests are failing for the existing work in the patch and they need to be fixed.

I hope this helps.

Also one small thing, when you are finished working on an issue and you set the issue to "Needs review" unassign your name from the issue so people do not think you have assigned the issue to yourself for reviewing. People can see it that way.

Looking forward to seeing more patches :)

saki007ster’s picture

@emma.maria Thanks for guiding, i'll update the issue in sometime.

saki007ster’s picture

@emma.maria I have used the patch in #25 and updated it with my code, but some of the tests were failing locally so I did some changes in the tests as well. If possible can you review the changes which I have made.

saki007ster’s picture

Assigned: saki007ster » Unassigned
Status: Needs work » Needs review
mortendk’s picture

Status: Needs review » Needs work

The #36 patch looks like it forgot the purpose of this issue ;)
The block templates needs to be moved to classy (block.html.twig, block-list.html.twig) and they need to be cleaned off classes inside core - as all the previous patches have done before.

lauriii’s picture

Thank you @saki007ster for helping on the issue! I think the patch is more or less correct. It only misses the copied templates to classy theme. You can add them to the patch by running git add core and then when you are creating the patch you run git diff --cached :))

lauriii’s picture

mortendk’s picture

Status: Needs review » Needs work

block.html.twig in core is not cleaned up

mortendk’s picture

Status: Needs work » Needs review
FileSize
5.53 KB

removed classes from block template in core.

Status: Needs review » Needs work

The last submitted patch, 42: copy_block_and-2349625-42.patch, failed testing.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
git apply -v copy_block_and-2349625-42.patch
Checking patch core/modules/block/src/Tests/BlockViewBuilderTest.php...
error: while searching for:
    $request->setMethod('GET');

    $default_keys = array('entity_view', 'block', 'test_block', 'en', 'cache_context.theme');
    $default_tags = array('block_view', 'block:test_block', 'theme:stark', 'block_plugin:test_cache');

    // Advanced: cached block, but an alter hook adds an additional cache key.
    $this->setBlockCacheConfig(array(

error: patch failed: core/modules/block/src/Tests/BlockViewBuilderTest.php:215
error: core/modules/block/src/Tests/BlockViewBuilderTest.php: patch does not apply
Checking patch core/modules/block/templates/block-list.html.twig...
Checking patch core/modules/block/templates/block.html.twig...
Checking patch core/modules/block_content/templates/block-content-add-list.html.twig...
Checking patch core/modules/block/templates/block-list.html.twig => core/themes/classy/templates/block-list.html.twig...
Checking patch core/modules/block/templates/block.html.twig => core/themes/classy/templates/block.html.twig...

Rerolling...

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.93 KB

Rerolled. Will work on the failing tests once the bot chews on this one.

Status: Needs review » Needs work

The last submitted patch, 45: copy_block_and-2349625-45.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
8.28 KB

Here is a proper patch with block.html.twig actually moved to classy...

This also includes an attempt to fix the failing tests on Drupal\block\Tests\BlockViewBuilderTest.

Status: Needs review » Needs work

The last submitted patch, 47: copy_block_and-2349625-47.patch, failed testing.

davidhernandez’s picture

It looks like you deleted the templates from the core block module?

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
7.04 KB

Right! Don't know what I was thinking, thanks for taking a look @davidhernandez.

The attached patch passes all tests on my local install, but I'm getting an exception, not sure why...

Status: Needs review » Needs work

The last submitted patch, 50: copy_block_and-2349625-50.patch, failed testing.

mortendk’s picture

+++ b/core/modules/block/templates/block.html.twig
@@ -34,13 +34,7 @@
-{%

+++ b/core/themes/classy/templates/block.html.twig
@@ -0,0 +1,46 @@
+<div>

this template is missing all the classes as classy should have ? - think you copied the wrong file or cleaned it and then copied ;)

mortendk’s picture

removed block-content template, it should be its own patch - lets keep this as small as pissible
added the classes back for classy

mortendk’s picture

Status: Needs work » Needs review

go bot gogo

Status: Needs review » Needs work

The last submitted patch, 53: copy_block_and-2349625-53.patch, failed testing.

lauriii’s picture

We cannot change the tests to match core. We should instead fix the actual problem which is that the test is loading the template from block module instead of loading it from classy.

mortendk’s picture

Status: Needs work » Needs review
FileSize
5 KB

fixed the test so its actually testing on the class block block-block-test, but it do looks like your saying laurii that its not testing on classy theme, but it test on core, as theres no classname on the test :( (

)

Status: Needs review » Needs work

The last submitted patch, 57: copy_block_and-2349625-57.patch, failed testing.

dawehner’s picture

Partially its odd that we change the tests itself, honestly. Don't we kinda want to ensure that the templates with classes works?

mortendk’s picture

@dawehner - yup spotted that and put the test on the classes in again, cause else it makes no sense to test - its in #57
problem is that the test is stil done on stark not against classy :/
so looks like we need a new issue

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
6.23 KB

The patch attached properly enables classy in kerneltestbase - the test still fails but I think it will now be easier to fix.

Status: Needs review » Needs work

The last submitted patch, 61: 2349625.61.patch, failed testing.

Manuel Garcia’s picture

Oops... I guess I was more tired last night than I thought, sorry for the noise, and glad to see it's being pushed forward -=]

lauriii’s picture

Status: Needs work » Postponed

Created issue to make Kernel tests use Classy #2409811: Kernel tests should explicitly install themes. Postponing this issue till that one have been solved.

alexpott’s picture

Status: Postponed » Needs review
FileSize
2.67 KB
4.57 KB

Rerolled on top of #2409811: Kernel tests should explicitly install themes to prove that change works.

Status: Needs review » Needs work

The last submitted patch, 65: 2349625.65.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
3.34 KB

The changes to BlockSystemBrandingTest are unnecessary - this is a web test base test and is using classy - if we did have to change that test then we are doing something wrong.

mortendk’s picture

+++ b/core/modules/block/src/Tests/BlockViewBuilderTest.php
@@ -83,7 +83,7 @@ public function testBasicRendering() {
-    $expected[] = '<div id="block-test-block1" class="block block-block-test">';

dont we wanna test on these classes ? they should be expected to be there in classy ?

alexpott’s picture

re #68 nope - that test is a kerneltestbase test - there is no theme - it is just using the templates from block - which is (imo) correct.

mortendk’s picture

@alex so its using the block template from core/block/block.html.twig ?
... isnt that using "stark" or did i miss something :)

alexpott’s picture

@mortendk there is no theme at this point :) so it is just using what it has so that is the templates provided by the block module.

mortendk’s picture

@alex ok not sure i understand buuuuuuut whatever ;) - looks like were ready to rtbc.

alexpott’s picture

@mortendk KernelTestBase tests are special flowers :)

alexpott’s picture

FileSize
2.53 KB

Rerolled now the blocker is in.

davidhernandez’s picture

There is definitely css attached to those classes, particularly the layout ones. I don't see any discussion of it in the issue. Did anyone check that the Stark view of things was reasonably ok with the classes removed?

mortendk’s picture

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

we are not moving css yet - its been agreed that we fixed that after we have moved templates to classy, to keep it simple.

layout.css in stark have references to a .block class:

 .layout-sidebar-second .block {
    float: left; /* LTR */
    width: 33%;
  }
  [dir="rtl"] .layout-sidebar-second .block {
    float: right;
  }
  .layout-sidebar-second .block:nth-child(3n+1) {
    clear: both;
  }

As all these classes should be moved from core to classy - When thats done we will as a part of classy + bartik + seven css cleanup.

I have created a follow up issue for this, so we dont forget to clean this up #2418903: clean up css for blocks & layouts, classy - stark-

blocks are still working fine in stark:

mortendk’s picture

Title: Copy block and block_content templates to Classy » Copy block templates to Classy

change title to refer to what were actually doing here.

lauriii’s picture

Are we sure we want to remove the {{ attributes }} or should we leave it & just remove classes from template?

davidhernandez’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/templates/block.html.twig
@@ -34,13 +34,7 @@
-<div{{ attributes.addClass(classes) }}>

This line. Lauri is right. The attribute print is suppose to stay. Only the addClass part should get removed.

mortendk’s picture

Status: Needs work » Needs review
FileSize
2.55 KB

Added back the {{ attributes }}

Status: Needs review » Needs work

The last submitted patch, 80: copy_block_templates_to-2349625-80.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
2.59 KB

*shakes fist at testbot* fixed the test ;)
GO BOT GO!

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

#78 fixd

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 4691ba9 on
    Issue #2349625 by mortendk, alexpott, lauriii, Manuel Garcia,...

Status: Fixed » Closed (fixed)

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