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
HTML classes should be usually added in templates instead of preprocess functions.
Proposed resolution
Move the HTML classes from the preprocess function to the template. The classes should be added in a template textarea.html.twig file.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#49 | 3105575-49.patch | 1.98 KB | bnjmnm |
#49 | interdiff_46-49.txt | 516 bytes | bnjmnm |
#46 | 3105575-46.patch | 2.48 KB | himanshu_sindhwani |
#43 | 3105575-43.patch | 2.48 KB | himanshu_sindhwani |
#32 | BeforePatch.png | 260.35 KB | priyanka.sahni |
Comments
Comment #2
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedKindly apply a new patch.
Comment #3
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #4
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #5
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedI have tested the patch and looks good to be merged.
The classes are being added from textarea.hrml.twig file.
Adding the screenshot below the. The classes are being highlighted which are in the textarea.html.twig file.
Comment #6
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #7
siddhant.bhosale CreditAttribution: siddhant.bhosale as a volunteer and at QED42 commentedComment #8
lauriiiIt seems like this template is removing some classes from the templates. For example
form-textarea-wrapper
class is now missing which is used by Claro.Comment #9
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented@lauriii, i have rechecked that form-textarea-wrapper class is not missing after adding template. Can you help me in which case it is getting lost.
Comment #10
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #11
lauriii@Hardik_Patel_12 based on your patch the screenshot has been taken on Seven theme. This change is in Claro.
Comment #12
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedKindly review a new patch.
Comment #13
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedComment #14
lauriiiDid you check if we should keep any of the classes that already exist in Classy textarea.html.twig?
Comment #15
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedClassy textarea.html.twig has added classes as follows
set classes = [
'form-textarea',
resizable ? 'resize-' ~ resizable,
required ? 'required',
]
so here it is adding 2 extra classes called 'resize-' ~ resizable and required.
So we need to add these classes in claro textarea.html.twig also?.
I have checked that resize-none, resize-vertical , resize-horizontal , resize-both classes are only used by stable theme only.
Comment #17
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedI've re-rolled patch for 9.1
Comment #19
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedplease avoid #17
re-rolled patch for 9.1
Comment #20
IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commentedHi @kishor_kolekar thanks for the patch #19
It working as expected.
Comment #21
IndrajithKB CreditAttribution: IndrajithKB at Srijan | A Material+ Company for Drupal India Association commentedComment #22
xjmComment #23
lauriiiIt seems like Claro depends on those classes because there is some CSS attached to them, at least in Stable/core. Let's make sure that the CSS classes from
core/themes/classy/templates/form/textarea.html.twig
are included in the new template, and thatcore/themes/classy/templates/form/textarea.html.twig
gets removed.Comment #24
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedComment #25
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedHi @lauriii,
I've added those classed from
core/themes/classy/templates/form/textarea.html.twig
into new template and removed it from classy section.Please review this patch.
Comment #26
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedComment #28
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedComment #29
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedComment #30
pradeepjha CreditAttribution: pradeepjha at Srijan | A Material+ Company for Drupal India Association commentedComment #31
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #32
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedVerified and tested by applying the patch #29.It looks good to me.Can be moved to RTBC. RTBC +1.
Before Patch -
After Patch -
Comment #33
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #34
lauriiiThis requires a post update hook because we are moving a template. It doesn't seem like there's one in Drupal 9.1.x yet, so we should add one here to make sure that people can upgrade to 9.1.x without getting errors.
Comment #35
naresh_bavaskarComment #36
naresh_bavaskarComment #37
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedComment #38
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedImplementing the ask in #34, Please review. The patch is made by taking latest changes from the branch.
Comment #39
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedComment #40
lauriiiThe patch in #38 doesn't include the new template.
Comment #41
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commented@laurii, this is because the latest pull already had those changes. Therefore they didn’t show up when i created the patch.
Comment #42
lauriii@himanshu_sindhwani I cannot see
core/themes/claro/templates/form/textarea.html.twig
in the code base at least on the 9.1.x branch. Did you by any chance compare against a local development branch?Comment #43
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedYes, I see it was due to some issues in my local branch. Thanks for the review @laurii.
I have updated the patch with a template file missing in #38. Please check.
Comment #44
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedComment #46
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedCorrecting the failure in #43.
Comment #47
himanshu_sindhwani CreditAttribution: himanshu_sindhwani at Srijan | A Material+ Company for Drupal India Association commentedComment #48
bnjmnmThe changes to claro.theme and the template look good, just spotted one thing:
A recently committed issue added
system_post_update_claro_dropbutton_variants()
to 9.1, so this empty post-update hook is no longer needed.Comment #49
bnjmnmSince removing the post_update hook implementation is fairly straightforward, I'm providing a patch and an RTBC.
Comment #51
lauriiiCommitted 4b052b5 and pushed to 9.1.x. Thanks!