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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hardik_Patel_12 created an issue. See original summary.

Hardik_Patel_12’s picture

Kindly apply a new patch.

Hardik_Patel_12’s picture

Assigned: Hardik_Patel_12 » Unassigned
Status: Needs work » Needs review
siddhant.bhosale’s picture

Assigned: Unassigned » siddhant.bhosale
siddhant.bhosale’s picture

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

siddhant.bhosale’s picture

Assigned: siddhant.bhosale » Unassigned
siddhant.bhosale’s picture

Status: Needs review » Reviewed & tested by the community
lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/claro/templates/form/textarea.html.twig
@@ -0,0 +1,25 @@
+<div{{ wrapper_attributes }}>
+  <textarea{{ attributes.addClass(classes) }}>{{ value }}</textarea>

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

Hardik_Patel_12’s picture

FileSize
409.12 KB

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

Hardik_Patel_12’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Needs work

@Hardik_Patel_12 based on your patch the screenshot has been taken on Seven theme. This change is in Claro.

Hardik_Patel_12’s picture

FileSize
1.6 KB
170 bytes

Kindly review a new patch.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
lauriii’s picture

Did you check if we should keep any of the classes that already exist in Classy textarea.html.twig?

Hardik_Patel_12’s picture

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

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kishor_kolekar’s picture

I've re-rolled patch for 9.1

Status: Needs review » Needs work

The last submitted patch, 17: 3105575-17.patch, failed testing. View results

kishor_kolekar’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

please avoid #17
re-rolled patch for 9.1

IndrajithKB’s picture

FileSize
61.5 KB

Hi @kishor_kolekar thanks for the patch #19
It working as expected.

issue after patch applied

IndrajithKB’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs frontend framework manager review

It 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 that core/themes/classy/templates/form/textarea.html.twig gets removed.

pradeepjha’s picture

Assigned: Unassigned » pradeepjha
pradeepjha’s picture

Hi @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.

pradeepjha’s picture

Assigned: pradeepjha » Unassigned
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: 3105575-25.patch, failed testing. View results

pradeepjha’s picture

Assigned: Unassigned » pradeepjha
pradeepjha’s picture

Assigned: pradeepjha » Unassigned
FileSize
1.98 KB
1.73 KB
pradeepjha’s picture

Status: Needs work » Needs review
priyanka.sahni’s picture

Assigned: Unassigned » priyanka.sahni
priyanka.sahni’s picture

FileSize
379.68 KB
260.35 KB

Verified and tested by applying the patch #29.It looks good to me.Can be moved to RTBC. RTBC +1.

Before Patch -
BeforePatch

After Patch -
After Patch

priyanka.sahni’s picture

Assigned: priyanka.sahni » Unassigned
lauriii’s picture

Status: Needs review » Needs work

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

naresh_bavaskar’s picture

Assigned: Unassigned » naresh_bavaskar
naresh_bavaskar’s picture

Assigned: naresh_bavaskar » Unassigned
himanshu_sindhwani’s picture

Assigned: Unassigned » himanshu_sindhwani
himanshu_sindhwani’s picture

Implementing the ask in #34, Please review. The patch is made by taking latest changes from the branch.

himanshu_sindhwani’s picture

Assigned: himanshu_sindhwani » Unassigned
Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Needs work

The patch in #38 doesn't include the new template.

himanshu_sindhwani’s picture

@laurii, this is because the latest pull already had those changes. Therefore they didn’t show up when i created the patch.

lauriii’s picture

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

himanshu_sindhwani’s picture

Yes, 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.

himanshu_sindhwani’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 43: 3105575-43.patch, failed testing. View results

himanshu_sindhwani’s picture

Correcting the failure in #43.

himanshu_sindhwani’s picture

Status: Needs work » Needs review
bnjmnm’s picture

Status: Needs review » Needs work

The changes to claro.theme and the template look good, just spotted one thing:

+++ b/core/modules/system/system.post_update.php
@@ -168,3 +168,11 @@ function system_post_update_schema_version_int() {
+function system_post_update_remove_claro_preprocess() {
+  // Empty post-update hook.
+}

A recently committed issue added system_post_update_claro_dropbutton_variants() to 9.1, so this empty post-update hook is no longer needed.

bnjmnm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
516 bytes
1.98 KB

Since removing the post_update hook implementation is fairly straightforward, I'm providing a patch and an RTBC.

  • lauriii committed 4b052b5 on 9.1.x
    Issue #3105575 by himanshu_sindhwani, Hardik_Patel_12, pradeepjha,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4b052b5 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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