(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/filter/templates/filter-caption.html.twig
core/modules/filter/templates/filter-guidelines.html.twig
core/modules/filter/templates/filter-tips.html.twig
core/modules/filter/templates/text-format-wrapper.html.twig

CommentFileSizeAuthor
#29 interdiff-2349677.txt894 bytesdavidhernandez
#29 copy_filter_templates-2349677-29.patch11.95 KBdavidhernandez
#27 copy_filter_templates-2349677-27.patch11.08 KBdavidhernandez
#20 interdiff.txt11.62 KBxen
#20 copy_filter_templates-2349677-20.patch11.08 KBxen
#8 copy_filter_templates-2349677-8.patch7.66 KBrunand
#2 filter-in-classy-2349677-2.patch7.68 KBemma.maria

Comments

emma.maria’s picture

emma.maria
she/her
commented
Assigned: Unassigned » emma.maria
emma.maria’s picture

emma.maria
she/her
commented
Assigned: emma.maria » Unassigned
Issue tags: +cssbanana
StatusFileSize
new filter-in-classy-2349677-2.patch7.68 KB

I have copied the templates over to classy and removed the classes from the original module templates. There is CSS left in the module that reference the removed classes so a follow up is needed - tagged cssbanana.

derheap’s picture

derheap commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: filter-in-classy-2349677-2.patch, failed testing.

derheap’s picture

derheap commented

We should remove all classes, but attributes should still be printed.
Otherwise RDF and other things can break.

  1. +++ b/core/modules/filter/templates/filter-guidelines.html.twig
@@ -19,13 +19,7 @@
-<div{{ attributes.addClass(classes) }}>
-  <h4 class="label">{{ format.name }}</h4>
+<div>

    {{attributs}} should still be printed. 

  2. +++ b/core/modules/filter/templates/filter-tips.html.twig
@@ -24,30 +24,19 @@
-      <div{{ tip.attributes.addClass(tip_classes) }}>
+      <div>

    {{tip.attributes}} should still be printed. 

  3. +++ b/core/modules/filter/templates/filter-tips.html.twig
@@ -24,30 +24,19 @@
-        <li{{ item.attributes.addClass(item_classes) }}>{{ item.tip }}</li>
+        <li>{{ item.tip }}</li>

    {{item.attributes}} should still be printed. 

  4. +++ b/core/modules/filter/templates/text-format-wrapper.html.twig
@@ -15,14 +15,9 @@
-    <div{{ attributes.addClass(classes) }}>{{ description }}</div>
+    <div>{{ description }}</div>

    {{attributes}} should still be printed.

mortendk’s picture

mortendk commented
Issue tags: +drupalhagen
runand’s picture

runand commented
Assigned: Unassigned » runand
runand’s picture

runand commented
Assigned: runand » Unassigned
Status: Needs work » Needs review
StatusFileSize
new copy_filter_templates-2349677-8.patch7.66 KB

Status: Needs review » Needs work

The last submitted patch, 8: copy_filter_templates-2349677-8.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

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

lauriii queued 8: copy_filter_templates-2349677-8.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: copy_filter_templates-2349677-8.patch, failed testing.

xen’s picture

xen commented
Assigned: Unassigned » xen
xen’s picture

xen commented
Status: Needs work » Needs review

Checked the JS files using Stark:

filter.filter_html.admin.js
- Checked that the "Allowed HTML tags" text field got updated when dragging the sup button into the CKEditor toolbar, and that the description explained that the tag was added.

filter.admin.js
- Checked that the settings for a configurable filter was properly shown and hidden when toggling the checkbox.

filter.js
- Checked that the format description changed when changing text format on a new article.

Everything worked the same on Bartik and Stark.

The failures of the testbot is differences in classes on some elements, but I believe that the bot should now test using Classy as a theme, so I'm requeing.

xen’s picture

xen commented
Assigned: xen » Unassigned

Xen queued 8: copy_filter_templates-2349677-8.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: copy_filter_templates-2349677-8.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

The test failures on the first group are unit tests that are checking for exact markup. We can probably just remove the classes from the test, as they don't appear to be relevant to the individual tests.

The main issue is those align-left, align-right, align-center classes might be considered functional, and it is exactly what those tests are checking for. Is it odd that it is a unit test and not a web test (which is why it is failing)?

If those classes are considered functional, we shouldn't remove them. Opinions?

xen’s picture

xen commented

Grepping for the classes finds them in system.module.css.

I'd call them functional.

It does make some kind of sense that it's unit tests. It's testing the filtering function, and as little as possible else. That there's a bit of templating involved is a minor detail.

xen’s picture

xen commented
Status: Needs work » Needs review
StatusFileSize
new copy_filter_templates-2349677-20.patch11.08 KB
new interdiff.txt11.62 KB

Removed the classes from the test, apart from the align classes.
Re-introduced adding classes to the module filter-caption.html.twig, so the align classes will get properly added.

The interdiff seems a bit odd, I think git is to clever for it.

lauriii queued 20: copy_filter_templates-2349677-20.patch for re-testing.

mortendk’s picture

mortendk commented
Status: Needs review » Reviewed & tested by the community

The css needs to have classnames renamed so it follow the oocss doc standards.- that work can first be done when everything is moved over and should be a followup issue.

filter.caption.css contains elements that are essential for the functionality, and should be trated as such.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: copy_filter_templates-2349677-20.patch, failed testing.

mortendk queued 20: copy_filter_templates-2349677-20.patch for re-testing.

The last submitted patch, 20: copy_filter_templates-2349677-20.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Issue tags: +Needs reroll
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new copy_filter_templates-2349677-27.patch11.08 KB

In filter-guidelines 'format.name' changed to 'format.label'. That was the only change.

<h4 class="label">{{ format.label }}</h4>

Status: Needs review » Needs work

The last submitted patch, 27: copy_filter_templates-2349677-27.patch, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs work » Needs review
StatusFileSize
new copy_filter_templates-2349677-29.patch11.95 KB
new interdiff-2349677.txt894 bytes

Had to remove the class from tests.

mortendk’s picture

mortendk commented
Status: Needs review » Reviewed & tested by the community

ok good from here

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1b9e6bd on 8.0.x 
    Issue #2349677 by davidhernandez, Xen, emma.maria, runand, mortendk:...

Status: Fixed » Closed (fixed)

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