(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/color/templates/color-scheme-form.html.twig

CommentFileSizeAuthor
#16 Screen Shot 2015-01-18 at 13.43.21.jpg386.43 KBLewisNyman
#16 Screen Shot 2015-01-18 at 13.43.01.jpg432.09 KBLewisNyman
#16 Screen Shot 2015-01-18 at 13.42.35.jpg352.75 KBLewisNyman
#14 color-to-classy-2349653-14.patch2.17 KBbradwade
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,101 pass(es). View
#11 color-to-classy-2349653-11.patch2.15 KBbradwade
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,101 pass(es). View
#8 copy_color_templates_to-2349653-8.patch1.55 KBrunand
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,640 pass(es). View
#2 color-to-classy-2349653-2.patch1.56 KBemma.maria
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,904 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

emma.maria’s picture

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

Assigned: emma.maria » Unassigned
Status: Active » Needs review
FileSize
1.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,904 pass(es). View

I 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.

emma.maria’s picture

Issue tags: +cssbanana

Tagged 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.

mortendk’s picture

Status: Needs review » Needs work
+++ b/core/modules/color/templates/color-scheme-form.html.twig
@@ -15,9 +15,9 @@
-<div class="color-form clearfix">

.color-form is used by the js, so it should be prefixed with js ?

wonder how much we win with this patch & color module ?

davidhernandez’s picture

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

mortendk’s picture

Issue tags: +drupalhagen
runand’s picture

Assigned: Unassigned » runand
runand’s picture

Assigned: runand » Unassigned
Status: Needs work » Needs review
FileSize
1.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,640 pass(es). View
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.

bradwade’s picture

Assigned: Unassigned » bradwade
Status: Needs review » Needs work

Yes, 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:

  • Apply latest patch.
  • Copy core/themes/bartik/color to core/themes/stark/color to enable the color form.
  • Enable Stark as admin theme.
  • Navigate to admin/appearance/settings/stark to see the form.

I will look more into this.

bradwade’s picture

FileSize
2.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,101 pass(es). View

I 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.

bradwade’s picture

Status: Needs work » Needs review
bradwade’s picture

While 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.

bradwade’s picture

FileSize
2.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,101 pass(es). View

Adding "color-form" class back in for reasons stated in #13.

LewisNyman’s picture

Assigned: bradwade » Unassigned
Issue tags: +Needs screenshots

Thanks for working on this, I think we need some screenshots to show that nothing is broken.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
FileSize
352.75 KB
432.09 KB
386.43 KB

Here 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

alexpott’s picture

Status: Reviewed & tested by the community » Postponed
mortendk’s picture

dont 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]

lauriii’s picture

Component: theme system » Classy theme
joelpittet’s picture

Version: 8.0.x-dev » 9.x-dev

Automated triage: bumping to 9.0.x because our API base themes (stable and classy) are locked in with the release of 8.0.x.

catch’s picture

Version: 9.x-dev » 8.3.x-dev
Priority: Normal » Minor

Moving back to 8.3.x and minor per #2349661: Copy config_translation templates to Classy.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.