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/color/templates/color-scheme-form.html.twig
Comment | File | Size | Author |
---|---|---|---|
#16 | Screen Shot 2015-01-18 at 13.43.21.jpg | 386.43 KB | LewisNyman |
#16 | Screen Shot 2015-01-18 at 13.43.01.jpg | 432.09 KB | LewisNyman |
#16 | Screen Shot 2015-01-18 at 13.42.35.jpg | 352.75 KB | LewisNyman |
#14 | color-to-classy-2349653-14.patch | 2.17 KB | bradwade |
#11 | color-to-classy-2349653-11.patch | 2.15 KB | bradwade |
Comments
Comment #1
emma.mariaComment #2
emma.mariaI have copied the original Color template to classy/templates/color/color-scheme-form.html.twig.removed and removed the classes from the original module template.
There is CSS that references one of the removed classes in the template file.
However it would be good to have some more guidance from @davidhernandez or @mortendk on where exactly you want us to put the CSS in the other base themes.
Comment #3
emma.mariaTagged this issue as it needs a follow up for the CSS referencing the removed classes in the original module template file - tagged cssbanana via @mortendk request.
Comment #4
mortendk CreditAttribution: mortendk commented.color-form is used by the js, so it should be prefixed with js ?
wonder how much we win with this patch & color module ?
Comment #5
davidhernandezThe templates should go into the root of the templates folder, not a subfolder.
Comment #6
mortendk CreditAttribution: mortendk commentedComment #7
runand CreditAttribution: runand commentedComment #8
runand CreditAttribution: runand commentedComment #9
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 #10
bradwade CreditAttribution: bradwade commentedYes, the .color-form class is being used by core/modules/color/color.js and, indeed, the functionality does not work with the admin theme set to Stark.
There are a couple of steps to reproduce:
I will look more into this.
Comment #11
bradwade CreditAttribution: bradwade commentedI created a new data attribute and updated the JavaScript to refer to it. Locally tested that color form's JS functionality was restored in Stark admin theme with these changes.
Comment #12
bradwade CreditAttribution: bradwade commentedComment #13
bradwade CreditAttribution: bradwade commentedWhile looking at what to do with the CSS include with the color module, I've determined that a good chunk of it is functional. Specifically it adds in the lock icon and places it to the right of each color field, making the empty div a clickable icon/button that links the field with the one above it. (Without this the div is empty and there is nothing to click on.) The rest of the css is mostly dealing with laying out the form/preview elements.
While the front-end of the color module needs some attention (i.e. stop styling with #ids in css and use data attributes in JS, implement Drupal standard BEM/SMACSS classes, reworking implementation of this lock icon and more) all of that is out of scope for this issue...
In this case, I'd recommend (1) leaving all of the CSS in the color module and (2) leave the .color-form class in the core template as well since there is functional CSS that depends on it.
This patch would still copy the template to Classy, remove some unnecessary clearfix classes, and have JS switch to using data-attribute from the class.
Comment #14
bradwade CreditAttribution: bradwade commentedAdding "color-form" class back in for reasons stated in #13.
Comment #15
LewisNyman CreditAttribution: LewisNyman commentedThanks for working on this, I think we need some screenshots to show that nothing is broken.
Comment #16
LewisNyman CreditAttribution: LewisNyman commentedHere are screenshots of it looking good in Stark, Classy, and Seven.
The locked/unlocked link is a bad issue, I am tempted to fix it here but I think we are better off tackling it in a follow up. - #2409653: The color module lock/unlock link is not accessible
Comment #17
alexpottPostponed on discussions in #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme
Comment #18
mortendk CreditAttribution: mortendk commenteddont understand this "dont move backend templates" to classy - classy is also a base for backend themes, so lets not be inconsistence in the way classy works.
i also commented on the [# #2348543]
Comment #19
lauriiiComment #20
joelpittetAutomated triage: bumping to 9.0.x because our API base themes (stable and classy) are locked in with the release of 8.0.x.
Comment #21
catchMoving back to 8.3.x and minor per #2349661: Copy config_translation templates to Classy.
Comment #34
quietone CreditAttribution: quietone at PreviousNext commentedAsked in #frontend and andy-blum replied that this should move to the contrib project.