Problem/Motivation

core template files should not contain any classes that are not essential for functionality

Proposed resolution

Remove all classes that are not essential to functionality
If a class is used by javascript it must eather be prefixed with .js- or change to a data-attribute as a selector instead.
We want a clean seperation between javascript & css.
If a test fails, make sure its not testing on stark but on classy's template

Twig Templates to modify

  • core/modules/system/templates/datetime-form.html.twig
  • core/modules/system/templates/datetime-wrapper.html.twig
  • core/modules/system/templates/details.html.twig
  • core/modules/system/templates/dropbutton-wrapper.html.twig

dropbutton-wrapper is moved to its own issue, as it also contains classerenaming etc see comment #30

Remaining tasks

Visual test on all renamed js- based classes.
rewrite Dropbuttons css so its not depening on js- and its seprated

    <div class="js-dropbutton-wrapper dropbutton-wrapper-theme">
      <div class="js-dropbutton-widget dropbutton-widget-theme">
....

about classnames
Separate style from behavior by using dedicated classes for JavaScript manipulation rather than relying on classes already in use for CSS. This way, we can modify classes for style purposes without fear of breaking JS, and vice versa. To make the distinction clear, classes used for JavaScript manipulation should be prefixed with 'js-'. These JavaScript hooks must never be used for styling purposes. See the section ‘Formatting Class Names’ for more information on naming conventions.
https://www.drupal.org/node/1887918#separate-concerns

User interface changes

API changes

See parent issue(s) and related issue(s) for details,

Files: 
CommentFileSizeAuthor
#37 interdiff-d-6-7.txt580 bytesmortendk
#37 d-7.diff1.54 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,364 pass(es). View
#1 issue-2407725-1.patch5.1 KBSivaji
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,945 pass(es), 3 fail(s), and 0 exception(s). View
#3 issue-2407725-3.patch4.04 KBSivaji
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,883 pass(es), 6 fail(s), and 0 exception(s). View
#7 core-d-template-cleanup.diff25.07 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,524 pass(es), 7 fail(s), and 0 exception(s). View
#11 Remove-classes-from-system-templates-d-2407725-11.patch423 bytesManjit.Singh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,799 pass(es). View
#12 code-d-template-interdiff.txt668 bytesmortendk
#13 core-d-template-cleanup-3.diff25.84 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,836 pass(es), 3 fail(s), and 0 exception(s). View
#19 template-d-4.diff26.6 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,887 pass(es). View
#21 Banners_and_Alerts_and_Content_types___drupal8_dev.png74.35 KBmortendk
#21 Content_types___drupal8_dev.png68.1 KBmortendk
#22 views-before.png599.87 KBrteijeiro
#22 views-after.png599.87 KBrteijeiro
#30 template-d-5.diff2.44 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,006 pass(es). View
#33 d-6.diff2.3 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,297 pass(es). View

Comments

Sivaji’s picture

Status: Active » Needs review
FileSize
5.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,945 pass(es), 3 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1: issue-2407725-1.patch, failed testing.

Sivaji’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,883 pass(es), 6 fail(s), and 0 exception(s). View

Restoring the details-wrapper class used in details.html.twig.

Status: Needs review » Needs work

The last submitted patch, 3: issue-2407725-3.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Postponed

Postponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the Classy theme for updates. Thanks.

davidhernandez’s picture

Title: Copy system templates d*.html.twig to Classy » Remove classes from system templates d*.html.twig
Issue summary: View changes
Status: Postponed » Needs work

Un-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.

mortendk’s picture

Status: Needs work » Needs review
FileSize
25.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,524 pass(es), 7 fail(s), and 0 exception(s). View

.details-wrapper is used by views-ui so not relevant (views-ui not copied to classy so no need for seperation)
.details-descriptionis purely cosmetic
.dropbutton-widget is build on js the classname is changed to js-* to make it clear for the themer that this is a class thats used by js.

Status: Needs review » Needs work

The last submitted patch, 7: core-d-template-cleanup.diff, failed testing.

mortendk’s picture

Issue summary: View changes
Manjit.Singh’s picture

@mortendk Looking into #3 patch, Everything seems fine but Why automate test fails ?

Manjit.Singh’s picture

FileSize
423 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,799 pass(es). View

Remove classes from datetime-form.html.twig

mortendk’s picture

Status: Needs work » Needs review
FileSize
668 bytes

@manjit: the "container-inline" class in datetime-form.html.twig was removed in the previous patch, also you forgot the rest of the patch ;)

I added a missing test for js-dropbotton, see interdiff

mortendk’s picture

FileSize
25.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,836 pass(es), 3 fail(s), and 0 exception(s). View

works better when the patch is uploaded ;)

Status: Needs review » Needs work

The last submitted patch, 13: core-d-template-cleanup-3.diff, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: core-d-template-cleanup-3.diff, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: core-d-template-cleanup-3.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
26.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,887 pass(es). View

set test to classy

Status: Needs review » Needs work

The last submitted patch, 19: template-d-4.diff, failed testing.

mortendk’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
74.35 KB
68.1 KB

Cant reproduce on my local everytest passes there, set to retest

Screenshots for dropbotton:

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
599.87 KB
599.87 KB

@mortendk is right. That tests are wrong even before applying this patch. I think it's a RTBC, check screenshots:

Views BEFORE

Views AFTER

davidhernandez queued 19: template-d-4.diff for re-testing.

davidhernandez’s picture

You are rtbc-ing a red patch. We have to have it come back green. If it is passing locally, but failing on testbot, that is something we need to look into.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/misc/dropbutton/dropbutton.css
@@ -87,15 +87,15 @@
-.js .dropbutton-multiple .dropbutton-widget {
+.js .dropbutton-multiple .js-dropbutton-widget {
...
-.js[dir="rtl"] .dropbutton-multiple .dropbutton-widget {
+.js[dir="rtl"] .dropbutton-multiple .js-dropbutton-widget {

This makes me even more reserved about adding js- to classes that are in the template. Sure if javascript adds the class and does whatever.

mortendk’s picture

js- classes must be added if a class is used for js, but what we might have to do is now refactor all of the css out of this mess & add in new classes etc.

I would suggest that we remove those files out of this commit so we can take each of the js infected files and clean them up, but dont hold the rest back ?

mortendk’s picture

Issue tags: +jsbanana
mortendk’s picture

Issue summary: View changes
Status: Needs review » Needs work

the dropbotton css component needs to be rewritten - would it make sense to move that to a seprate issue ?

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2428193: make dropbutton component follow css coding standards
FileSize
2.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,006 pass(es). View

I have looked at the dropbutton component, and have decidet to remove it from this issue - create a seperate issue for it. Theres to amny otherthings going on inside of dropbutton that potentially can drag on forever (test, classname bikeshed etc)
Dropbotton issue
#2428193: make dropbutton component follow css coding standards

New patch uploaded without dropbutton-wrapper.html.twig

joelpittet’s picture

+++ b/core/modules/system/templates/datetime-wrapper.html.twig
@@ -15,16 +15,10 @@
-    required ? 'form-required',

This is sketchy, removing this from core because it's functionality.

Everything else I'm cool with.

mortendk’s picture

Status: Needs review » Needs work

.form-required is used in /core/misc/states.js:

if i read it right it removes required with js from <foo required>?

$(document).on('state:required', function (e) {
    if (e.trigger) {
      if (e.value) {
        var $label = $(e.target).attr({'required': 'required', 'aria-required': 'aria-required'}).closest('.form-item, .form-wrapper').find('label');
        // Avoids duplicate required markers on initialization.
        if (!$label.hasClass('form-required').length) {
          $label.addClass('form-required');
        }
      }
      else {
        $(e.target).removeAttr('required aria-required').closest('.form-item, .form-wrapper').find('label.form-required').removeClass('form-required');
      }
    }
  });

looks like we have another case of .js-banana

mortendk’s picture

Status: Needs work » Needs review
Related issues: +#2408473: Rewrite form.css inline with our CSS standards
FileSize
2.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,297 pass(es). View

the js dependency on the classes needs to be fixed with all the other js / css issues in #2408473: Rewrite form.css inline with our CSS standards

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Ok, let's try to clean-up js classes in a different issue. Back to RTBC!

mortendk’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies, presumably because of #2378883: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests.

+++ b/core/modules/system/src/Tests/Common/RenderTest.php
@@ -581,6 +581,10 @@ function testDrupalRenderChildrenPostRenderCache() {
 
+    // Enable the Classy theme.
+    \Drupal::service('theme_handler')->install(['classy']);
+    $this->config('system.theme')->set('default', 'classy')->save();
+

This looks quite odd. Fortunately this hopefully won't be necessary anymore with that test now turned into unit tests instead.

mortendk’s picture

FileSize
1.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,364 pass(es). View
580 bytes
mortendk’s picture

Status: Needs work » Needs review

hurray ok lets see where this goes

DickJohnson’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and solves the issues mentioned by @webchich on #36.

ps. In future decent naming and .patch in the end of file would rock.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Changing templates is permitted in beta. Committed 44323cf and pushed to 8.0.x. Thanks!

  • alexpott committed 44323cf on 8.0.x
    Issue #2407725 by mortendk, sivaji@knackforge.com, rteijeiro, Manjit....

Status: Fixed » Closed (fixed)

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