Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
(First, verify that the preprocess changes have been made. #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.)
- Copy the Twig templates from the core module's templates directory to Classy's templates directory. Include all templates, even ones without classes.
- 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
Comment | File | Size | Author |
---|---|---|---|
#82 | copy_block_templates_to-2349625-82.patch | 2.59 KB | mortendk |
#80 | copy_block_templates_to-2349625-80.patch | 2.55 KB | mortendk |
#76 | Screen Shot 2015-02-02 at 23.05.24 .png | 72.51 KB | mortendk |
#74 | 2349625.74.patch | 2.53 KB | alexpott |
#67 | 2349625.67.patch | 3.34 KB | alexpott |
Comments
Comment #1
davidhernandezComment #2
mortendk CreditAttribution: mortendk commentedmove the classes outta core and into class
the discussion about which classnames we want in core should be done as a followup issue
Comment #4
emma.mariaComment #5
emma.mariaThe last patch had template files straight in the root of classy and not in a templates folder. I have fixed this :)
Comment #6
emma.mariaFurther 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/*
Comment #9
davidhernandezThe templates should go into the root of the templates folder, not a subfolder.
Comment #10
mortendk CreditAttribution: mortendk commentedmoved the templates to classy/templates
Comment #12
davidhernandezPlease 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.
Comment #15
mortendk CreditAttribution: mortendk commentedreroll
Comment #16
DickJohnson CreditAttribution: DickJohnson commentedComment #17
lauriiiComment #18
lauriiiThis patch is in wrong issue. Ignore
Comment #19
lauriiiRight patch finally
Comment #24
DickJohnson CreditAttribution: DickJohnson commentedComment #25
DickJohnson CreditAttribution: DickJohnson commentedTried to fix the tests.
Comment #26
DickJohnson CreditAttribution: DickJohnson commentedComment #28
DickJohnson CreditAttribution: DickJohnson commentedComment #29
DickJohnson CreditAttribution: DickJohnson commentedSorry, wrong issue.
Comment #30
saki007sterComment #31
saki007sterRemoved classes from the template files of block and block_content module.
Comment #32
saki007sterComment #34
emma.maria@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 :)
Comment #35
saki007ster@emma.maria Thanks for guiding, i'll update the issue in sometime.
Comment #36
saki007ster@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.
Comment #37
saki007sterComment #38
mortendk CreditAttribution: mortendk commentedThe #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.
Comment #39
lauriiiThank 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 rungit diff --cached
:))Comment #40
lauriiiComment #41
mortendk CreditAttribution: mortendk commentedblock.html.twig in core is not cleaned up
Comment #42
mortendk CreditAttribution: mortendk commentedremoved classes from block template in core.
Comment #44
Manuel Garcia CreditAttribution: Manuel Garcia commentedRerolling...
Comment #45
Manuel Garcia CreditAttribution: Manuel Garcia commentedRerolled. Will work on the failing tests once the bot chews on this one.
Comment #47
Manuel Garcia CreditAttribution: Manuel Garcia commentedHere 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
.Comment #49
davidhernandezIt looks like you deleted the templates from the core block module?
Comment #50
Manuel Garcia CreditAttribution: Manuel Garcia commentedRight! 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...
Comment #52
mortendk CreditAttribution: mortendk commentedthis template is missing all the classes as classy should have ? - think you copied the wrong file or cleaned it and then copied ;)
Comment #53
mortendk CreditAttribution: mortendk commentedremoved block-content template, it should be its own patch - lets keep this as small as pissible
added the classes back for classy
Comment #54
mortendk CreditAttribution: mortendk commentedgo bot gogo
Comment #56
lauriiiWe 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.
Comment #57
mortendk CreditAttribution: mortendk commentedfixed 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 :( (
Comment #59
dawehnerPartially its odd that we change the tests itself, honestly. Don't we kinda want to ensure that the templates with classes works?
Comment #60
mortendk CreditAttribution: mortendk commented@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
Comment #61
alexpottThe patch attached properly enables classy in kerneltestbase - the test still fails but I think it will now be easier to fix.
Comment #63
Manuel Garcia CreditAttribution: Manuel Garcia commentedOops... I guess I was more tired last night than I thought, sorry for the noise, and glad to see it's being pushed forward -=]
Comment #64
lauriiiCreated issue to make Kernel tests use Classy #2409811: Kernel tests should explicitly install themes. Postponing this issue till that one have been solved.
Comment #65
alexpottRerolled on top of #2409811: Kernel tests should explicitly install themes to prove that change works.
Comment #67
alexpottThe 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.
Comment #68
mortendk CreditAttribution: mortendk commenteddont we wanna test on these classes ? they should be expected to be there in classy ?
Comment #69
alexpottre #68 nope - that test is a kerneltestbase test - there is no theme - it is just using the templates from block - which is (imo) correct.
Comment #70
mortendk CreditAttribution: mortendk commented@alex so its using the block template from core/block/block.html.twig ?
... isnt that using "stark" or did i miss something :)
Comment #71
alexpott@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.
Comment #72
mortendk CreditAttribution: mortendk commented@alex ok not sure i understand buuuuuuut whatever ;) - looks like were ready to rtbc.
Comment #73
alexpott@mortendk KernelTestBase tests are special flowers :)
Comment #74
alexpottRerolled now the blocker is in.
Comment #75
davidhernandezThere 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?
Comment #76
mortendk CreditAttribution: mortendk commentedwe 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:
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:
Comment #77
mortendk CreditAttribution: mortendk commentedchange title to refer to what were actually doing here.
Comment #78
lauriiiAre we sure we want to remove the {{ attributes }} or should we leave it & just remove classes from template?
Comment #79
davidhernandezThis line. Lauri is right. The attribute print is suppose to stay. Only the addClass part should get removed.
Comment #80
mortendk CreditAttribution: mortendk commentedAdded back the {{ attributes }}
Comment #82
mortendk CreditAttribution: mortendk commented*shakes fist at testbot* fixed the test ;)
GO BOT GO!
Comment #83
lauriii#78 fixd
Comment #84
catchCommitted/pushed to 8.0.x, thanks!