Remove classes from system templates c*.html.twig

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

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

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

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

Tests updated.

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

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

do what it should do - clean core / visually tested

doooh i cant rtbc this i added first patch

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

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

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

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

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

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

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. :\

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

Still needs a change record.

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.

change record added

rerolled after #2349559: [meta] Discuss the organization of subfolders in Classy

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?

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

It looks good!

Container BEFORE

Container AFTER

+++ 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?

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.

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!

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

Agreed, https://www.drupal.org/node/2452321 does this.

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.

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

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

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

Minor reroll.

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.

Re-uploading patch for bot.

This looks like it would still be doable.

rerolled

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.

rerolled

We need to change core/themes/stable/templates/form/checkboxes.html.twig too.

Added new patch based on previous comment (#58)

The last patch failed to apply on 9.2, so re-rolled for 9.2

Fixed Custom Commands Failed, #67 please review the patch

+++ b/core/themes/stable/templates/form/checkboxes.html.twig
--- a/core/yarn.lock
+++ b/core/yarn.lock

Your patch have an updated yarn.lock.

#69 Re-created patch, Please review it

Added a patch from #65

@Sakthivel M Sorry didn't notice you already added a patch for it. :)

Hi
Since checkboxes.html.twig file of classy is updated in this ticket, I think we need to change md5_file value also.
Updating md5_file hash value as per latest patch.

Patch not applying giving error while trying to apply
Needs - Re-roll

Checking patch core/modules/system/templates/checkboxes.html.twig...
Checking patch core/modules/system/templates/container.html.twig...
Checking patch core/modules/views_ui/js/views-admin.es6.js...
error: while searching for:
.once('filterConfigSelectAll');
const $selectAllCheckbox = $selectAll.find('input[type=checkbox]');
const $checkboxes = $selectAll
.closest('.form-checkboxes')
.find(
'.js-form-type-checkbox:not(.js-form-item-options-value-all) input[type="checkbox"]',
);

error: patch failed: core/modules/views_ui/js/views-admin.es6.js:1084
error: core/modules/views_ui/js/views-admin.es6.js: patch does not apply
Checking patch core/modules/views_ui/js/views-admin.js...
error: while searching for:
var $context = $(context);
var $selectAll = $context.find('.js-form-item-options-value-all').once('filterConfigSelectAll');
var $selectAllCheckbox = $selectAll.find('input[type=checkbox]');
var $checkboxes = $selectAll.closest('.form-checkboxes').find('.js-form-type-checkbox:not(.js-form-item-options-value-all) input[type="checkbox"]');

if ($selectAll.length) {
$selectAll.show();

error: patch failed: core/modules/views_ui/js/views-admin.js:479
error: core/modules/views_ui/js/views-admin.js: patch does not apply
Checking patch core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php...
Checking patch core/themes/classy/templates/form/checkboxes.html.twig...
Checking patch core/themes/stable/templates/form/checkboxes.html.twig...

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-rolled patch #70, Also added few missing files, add updated CSS files. Not adding interdiff as patch #70, no longer applies to D11. please review

Think this is good (if something we still want to do)

Searched the repo for "form-checkboxes" and didn't find anything using that specific class.

#82 not applied on 11.x, Re-roll patch needed.

Re-roll & Convert Patch #82 into MR. Please have a look.

Reroll seems fine.

Sorry, but this needs a review of the change record. It was added in 2015 and does not mention classes being removed. Is it correct?

@quietone is correct.

This was filed ~10 months before Drupal 8 was released, any time during those 10 months would have been a fine time to take care of this.
Drupal 8+ has now been around for 8.5 years, and removing the classes now would be incredibly disruptive and cause many regressions. The intentions here were good, but any benefits they'd have yeilded are trival compared to the problems that would be introduced. Closing so nobody winds up devoting any more time to something that I (Frontend Framework Manager) would not approve.