(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

Files: 
CommentFileSizeAuthor
#29 interdiff-2349677.txt894 bytesdavidhernandez
#29 copy_filter_templates-2349677-29.patch11.95 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,085 pass(es). View
#27 copy_filter_templates-2349677-27.patch11.08 KBdavidhernandez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,080 pass(es), 3 fail(s), and 0 exception(s). View
#20 interdiff.txt11.62 KBXen
#20 copy_filter_templates-2349677-20.patch11.08 KBXen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch copy_filter_templates-2349677-20.patch. Unable to apply patch. See the log in the details link for more information. View
#8 copy_filter_templates-2349677-8.patch7.66 KBrunand
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,724 pass(es), 12 fail(s), and 0 exception(s). View
#2 filter-in-classy-2349677-2.patch7.68 KBemma.maria
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,966 pass(es), 17 fail(s), and 0 exception(s). View

Comments

emma.maria’s picture

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

Assigned: emma.maria » Unassigned
Issue tags: +cssbanana
FileSize
7.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,966 pass(es), 17 fail(s), and 0 exception(s). View

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

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

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

Issue tags: +drupalhagen
runand’s picture

Assigned: Unassigned » runand
runand’s picture

Assigned: runand » Unassigned
Status: Needs work » Needs review
FileSize
7.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,724 pass(es), 12 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 8: copy_filter_templates-2349677-8.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, 8: copy_filter_templates-2349677-8.patch, failed testing.

Xen’s picture

Assigned: Unassigned » Xen
Xen’s picture

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

Assigned: Xen » Unassigned

Status: Needs review » Needs work

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

davidhernandez’s picture

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

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

Status: Needs work » Needs review
FileSize
11.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch copy_filter_templates-2349677-20.patch. Unable to apply patch. See the log in the details link for more information. View
11.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.

mortendk’s picture

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.

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

davidhernandez’s picture

Issue tags: +Needs reroll
davidhernandez’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
11.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,080 pass(es), 3 fail(s), and 0 exception(s). View

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

Status: Needs work » Needs review
FileSize
11.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,085 pass(es). View
894 bytes

Had to remove the class from tests.

mortendk’s picture

Status: Needs review » Reviewed & tested by the community

ok good from here

webchick’s picture

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.