Problem/Motivation

Seperate core templates from non essential css classes.

Proposed resolution

modify templates:

  • core/modules/system/templates/checkboxes.html.twig
  • core/modules/system/templates/confirm-form.html.twig
  • core/modules/system/templates/container.html.twig

Rename .form-wrapper to .js-form-wrapper to follow css coding standards
#2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality

Remaining tasks

* visual test of renames classes
* test should be done on classy and stark
* test url:
/admin/config/regional/translate ( FILTER TRANSLATABLE STRINGS box)
/node/1/edit (sidebar)

User interface changes

API changes

Original report by @username

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

Twig Templates to modify

Files: 
CommentFileSizeAuthor
#49 remove_classes_from-2407723-45_0.patch3.08 KBaadrian
#46 remove_classes_from-2407723-45.patch3.08 KBmgifford
#45 interdiff-removecclasses-44to45.txt439 bytesdavidhernandez
#45 remove_classes_from-2407723-45.patch3.08 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,595 pass(es). View
#44 remove_classes_from-2407723-44.patch2.65 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,600 pass(es). View
#42 2407723-42-convert-form-checkbox-classes.patch2.64 KBmdrummond
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,547 pass(es). View
#34 container-before.png85.75 KBrteijeiro
#34 container-after.png85.34 KBrteijeiro
#18 quickedit.png141.69 KBrteijeiro
#15 stark-pre-patch.png55.43 KBmortendk
#15 stark-post-patch.png62.05 KBmortendk
#15 stark-node-edit-pre-patch.png107.51 KBmortendk
#15 stark-node-edit-post-patch.png113.58 KBmortendk
#15 stark-lang-transaltion-2.png35.57 KBmortendk
#15 stark-lang-transaltion-1.png52.17 KBmortendk
#15 patch-quick-edit.png35.84 KBmortendk
#15 patch-node-edit.png179.71 KBmortendk
#15 patch-node-edit-2.png189.29 KBmortendk
#15 patch-language-translation-2.png51.08 KBmortendk
#15 patch-language-translation-1.png42.58 KBmortendk
#8 2407723-remove-classes-update-tests-7.patch11.01 KBmherchel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,829 pass(es). View
#5 core-c-template-cleanup.diff6.6 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,280 pass(es), 4 fail(s), and 0 exception(s). View
#2 issue-2407723-1.patch2.18 KBSivaji
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,948 pass(es). View

Comments

Sivaji’s picture

Issue summary: View changes
Sivaji’s picture

Status: Active » Needs review
FileSize
2.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,948 pass(es). View
davidhernandez’s picture

Status: Needs review » 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 c*.html.twig to Classy » Remove classes from system templates c*.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
6.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,280 pass(es), 4 fail(s), and 0 exception(s). View

.form-wrapper in container.html.twig is used all over with loads of js dependencies, a rename of that css class to js-form-wrapperto follow the css standards.
If we dont rename this, then core files wont work as expexted.

Classes in use in system.theme .form-checkboxes, is also used in /views_ui/js/views-admin.js but should use classys checkbox.html.twig file (as we havent yet moved views_ui anywhere)

Status: Needs review » Needs work

The last submitted patch, 5: core-c-template-cleanup.diff, failed testing.

mherchel’s picture

I'm going to try to fix this right now, FYI

mherchel’s picture

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

Tests updated.

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue tags: +Needs screenshots

looks good to me but we need visual testing on this, so we dont do any regression

mortendk’s picture

Issue tags: +drupalcafe-feb2015
mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
mortendk’s picture

Screenshots from test

test on seven:
Filter can be open & closed with handy little form wrapper:

Edit node sidebar open/close form wrapper

Quick edit

Stark test


node edit
Pre patch:

post patch:

Quick edit -post patch

Quick edit

mortendk’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots, -drupalcafe-feb2015 +classy

do what it should do - clean core / visually tested

mortendk’s picture

Status: Reviewed & tested by the community » Needs review

doooh i cant rtbc this i added first patch

rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
141.69 KB

It looks good and works like in the screenshots provided by @mortendk. Good job!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2407723-remove-classes-update-tests-7.patch, failed testing.

mortendk’s picture

Status: Needs work » Reviewed & tested by the community

test is passed again #8 come on test bot -> settings this back to rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ b/core/misc/states.js
@@ -509,7 +509,7 @@
+        .closest('.form-item, .form-submit, .js-form-wrapper').toggleClass('form-disabled', e.value)

Are we going to also prefex form-item and form-sumbit with js-? I'm not entirely convinced by this change.

Also we should get a general change record for all the core class name changes.

mortendk’s picture

Are we going to also prefex form-item and form-sumbit with js-? I'm not entirely convinced by this change.

good point looks like we have to go through all forms and figure out how much .form-item & .form-submits are needed, & form-disabled for the js to work.
shakes fist at the js & css mashups

mortendk’s picture

Status: Needs work » Needs review
FileSize
2.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,341 pass(es). View

took a long hard look at this patch & the one for all the f* templates #2407727: Remove classes from system templates f*.html.twig, theres a big chunk of css & js dependencies that needs to be fixed.
i suggest that we clean what can be cleaned here for now (form-checkboxes) and in #2408473: Rewrite form.css inline with our CSS standards do the js & css dependency stuff.

this patch adds a js-form-checkboxes for the wrapper, for the selction of checkboxes & .form-checkboxes for the general styling, the style class is removed from crore template

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Ok, so keep this easy and clean-up "form-wrapper" class in a different step. Back to RTBC then...

mortendk’s picture

webchick’s picture

Assigned: Unassigned » alexpott

Back to Alex. However, after reading his concerns I'm now thinking I should probably not have committed #2426595: Rename indentation class to js-indentation. Hm. :\

davidhernandez’s picture

Indentation should be ok. It's whole purpose is functional, not style, and most of that change was JS.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Still needs a change record.

davidhernandez’s picture

I don't think this one in particular needs a change record. Didn't we compile that for something else, and create a doc page? I think it was part of then menu class name rewriting.

mortendk’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

change record added

mortendk’s picture

FileSize
2.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,706 pass(es). View
rteijeiro’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/templates/checkboxes.html.twig
    @@ -14,4 +14,4 @@
    +<div{{ attributes.addClass('js-form-checkboxes') }}>{{ children }}</div>
    

    Should we keep form-checkboxes class or...

  2. +++ b/core/themes/classy/templates/form/checkboxes.html.twig
    @@ -14,4 +14,4 @@
    +<div{{ attributes.addClass('js-form-checkboxes', 'form-checkboxes') }}>{{ children }}</div>
    

    Should we remove it here?

rteijeiro’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
85.34 KB
85.75 KB

Forget my last comment. Class should stay in Classy template and be removed in system template.

It looks good!

Container BEFORE

Container AFTER

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/views_ui/js/views-admin.js
@@ -775,7 +775,7 @@
-          $(this).parents('.form-checkboxes').find('input[type=checkbox]').each(function () {
+          $(this).parents('.js-form-checkboxes').find('input[type=checkbox]').each(function () {

I can't find where this behaviour is actually used. Anyone know?

mortendk’s picture

to be honest i have no idea - we found it and changed it.

my guess is somewhere are use can click on "select all checkboxes" but thats my best guess.

damiankloip’s picture

This is used on handler forms, E.g. entity bundle type 'select all' but we have just realised this is currently broken anyway, so will make a follow up issue to fix that!

alexpott’s picture

Re #37 considering this is currently broken - no can have tested what they've changed so we have to fix views behaviour first.

damiankloip’s picture

Cottser’s picture

Assigned: alexpott » Unassigned

It looks to me like this is now a duplicate of #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality, can someone please confirm? Looked at the patches in #8 and #32.

mortendk’s picture

dont looks like they are cleaning up & prefixing the form checkboxes ?
... or do i need new classes ?

mdrummond’s picture

FileSize
2.64 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,547 pass(es). View

Old patch, so rolled a fresh copy to get new eyes on this.

davidhernandez’s picture

Status: Needs review » Needs work
+++ b/core/themes/classy/templates/form/checkboxes.html.twig
@@ -12,4 +12,4 @@
+<div{{ attributes.addClass('js-form-checkboxes form-checkboxes') }}>{{ children }}</div>

Is this a change I guess we missed with the js-* prefix issues? Or did we only do form-item-*?

Also, missing removal from container.html.twig.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,600 pass(es). View

Minor reroll.

davidhernandez’s picture

FileSize
3.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,595 pass(es). View
439 bytes

Here is the container removal. We need to check this in Stark. There is some admin css looking for form-wrapper classes, but did not track down if it is specifically here.

mgifford’s picture

Re-uploading patch for bot.

Status: Needs review » Needs work

The last submitted patch, 46: remove_classes_from-2407723-45.patch, failed testing.

davidhernandez’s picture

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

This looks like it would still be doable.

aadrian’s picture

Status: Needs work » Needs review
FileSize
3.08 KB

rerolled

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mdrummond’s picture

aadrian: Thanks for the patch! It's really helpful if you can provide an interdiff to the previous patch so whoever is rerunning can see what changes have been made to the patch. If the patch is a reroll, just make sure to note that, because interdiffs don't make sense to do for rerolls.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.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.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.