Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|
Issue fork drupal-2407723
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedComment #2
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedComment #3
davidhernandezPostponing 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.
Comment #4
davidhernandezUn-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.
Comment #5
mortendk CreditAttribution: mortendk commented.form-wrapper
in container.html.twig is used all over with loads of js dependencies, a rename of that css class tojs-form-wrapper
to 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)Comment #7
mherchelI'm going to try to fix this right now, FYI
Comment #8
mherchelTests updated.
Comment #9
mortendk CreditAttribution: mortendk commentedComment #10
mortendk CreditAttribution: mortendk commentedComment #11
mortendk CreditAttribution: mortendk commentedlooks good to me but we need visual testing on this, so we dont do any regression
Comment #12
mortendk CreditAttribution: mortendk commentedComment #13
mortendk CreditAttribution: mortendk commentedComment #14
mortendk CreditAttribution: mortendk commentedComment #15
mortendk CreditAttribution: mortendk commentedScreenshots 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
Comment #16
mortendk CreditAttribution: mortendk commenteddo what it should do - clean core / visually tested
Comment #17
mortendk CreditAttribution: mortendk commenteddoooh i cant rtbc this i added first patch
Comment #18
rteijeiro CreditAttribution: rteijeiro commentedIt looks good and works like in the screenshots provided by @mortendk. Good job!
Comment #21
mortendk CreditAttribution: mortendk commentedtest is passed again #8 come on test bot -> settings this back to rtbc
Comment #22
alexpottAre we going to also prefex
form-item
andform-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.
Comment #23
mortendk CreditAttribution: mortendk commentedgood 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
Comment #24
mortendk CreditAttribution: mortendk commentedtook 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 templateComment #25
rteijeiro CreditAttribution: rteijeiro commentedOk, so keep this easy and clean-up "form-wrapper" class in a different step. Back to RTBC then...
Comment #26
mortendk CreditAttribution: mortendk commentedComment #27
webchickBack 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. :\
Comment #28
davidhernandezIndentation should be ok. It's whole purpose is functional, not style, and most of that change was JS.
Comment #29
alexpottStill needs a change record.
Comment #30
davidhernandezI 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.
Comment #31
mortendk CreditAttribution: mortendk commentedchange record added
Comment #32
mortendk CreditAttribution: mortendk commentedrerolled after #2349559: [meta] Discuss the organization of subfolders in Classy
Comment #33
rteijeiro CreditAttribution: rteijeiro commentedShould we keep form-checkboxes class or...
Should we remove it here?
Comment #34
rteijeiro CreditAttribution: rteijeiro commentedForget my last comment. Class should stay in Classy template and be removed in system template.
It looks good!
Container BEFORE
Container AFTER
Comment #35
alexpottI can't find where this behaviour is actually used. Anyone know?
Comment #36
mortendk CreditAttribution: mortendk commentedto 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.
Comment #37
damiankloip CreditAttribution: damiankloip commentedThis 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!
Comment #38
alexpottRe #37 considering this is currently broken - no can have tested what they've changed so we have to fix views behaviour first.
Comment #39
damiankloip CreditAttribution: damiankloip commentedAgreed, https://www.drupal.org/node/2452321 does this.
Comment #40
star-szrIt 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.
Comment #41
mortendk CreditAttribution: mortendk commenteddont looks like they are cleaning up & prefixing the form checkboxes ?
... or do i need new classes ?
Comment #42
RainbowArrayOld patch, so rolled a fresh copy to get new eyes on this.
Comment #43
davidhernandezIs 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.
Comment #44
davidhernandezMinor reroll.
Comment #45
davidhernandezHere 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.
Comment #46
mgiffordRe-uploading patch for bot.
Comment #48
davidhernandezThis looks like it would still be doable.
Comment #49
aadrian CreditAttribution: aadrian commentedrerolled
Comment #51
RainbowArrayaadrian: 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.
Comment #56
markhalliwellComment #57
kostyashupenkorerolled
Comment #58
alexpottWe need to change core/themes/stable/templates/form/checkboxes.html.twig too.
Comment #59
kostyashupenkoAdded new patch based on previous comment (#58)
Comment #65
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedThe last patch failed to apply on 9.2, so re-rolled for 9.2
Comment #67
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commentedFixed Custom Commands Failed, #67 please review the patch
Comment #68
imalabyaYour patch have an updated yarn.lock.
Comment #69
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commented#69 Re-created patch, Please review it
Comment #70
imalabyaAdded a patch from #65
Comment #71
imalabya@Sakthivel M Sorry didn't notice you already added a patch for it. :)
Comment #75
BS Pavan CreditAttribution: BS Pavan at Srijan | A Material+ Company commentedHi
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.
Comment #77
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedPatch 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...
Comment #80
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #82
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedRe-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
Comment #83
smustgrave CreditAttribution: smustgrave at Mobomo commentedThink 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.
Comment #86
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commented#82 not applied on 11.x, Re-roll patch needed.
Comment #87
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedComment #89
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll & Convert Patch #82 into MR. Please have a look.
Comment #90
smustgrave CreditAttribution: smustgrave at Mobomo commentedReroll seems fine.
Comment #91
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #92
quietone CreditAttribution: quietone at PreviousNext commentedSorry, but this needs a review of the change record. It was added in 2015 and does not mention classes being removed. Is it correct?
Comment #93
smustgrave CreditAttribution: smustgrave at Mobomo commented@quietone is correct.