Issue #2473955 by mortendk, rteijeiro, rachel_norfolk, Cottser, LewisNyman: Prefix form-wrapper classes with js-

Task

Prefix form-wrapper classes with js- to separate classes needed for JavaScript functionality from those used for styling and make it clear which classes can safely be removed without breaking JavaScript functionality.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing

Steps to test

@todo

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal
Unfrozen changes Unfrozen because it mostly just affects templates and JS which are not frozen.
Prioritized changes The main goal of this issue is to improve themer experience and arguably it reduces fragility where JavaScript and markup intersect.
Disruption Shouldn't be too disruptive as it is mostly affecting CSS classes that are added to markup. Themes extending Classy will only have classes added. Themes not extending Classy will have classes removed but they can be added back via template overrides.

User interface changes

None for themes extending Classy. Possible visual changes for other themes.

API changes

n/a

CommentFileSizeAuthor
#11 interdiff.txt847 bytesstar-szr
#11 prefix_form_wrapper-2473955-11.patch6 KBstar-szr
#10 prefix_form_wrapper-2473955-10.patch5.94 KBstar-szr
#1 2473955-form-wrapper-split.patch5.93 KBstar-szr

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2473955-form-wrapper-split.patch5.93 KB

Patch split from parent issue. I wasn't able to find anything additional by grepping.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work

I grepped in the core directory and couldn't find any other changes. I couldn't figure out where to test this change though.

+++ b/core/modules/views/src/Tests/ViewElementTest.php
@@ -71,7 +71,7 @@ public function testViewElement() {
-    $xpath = $this->xpath('//div[@class="views-element-container form-wrapper"]');
+    $xpath = $this->xpath('//div[@class="views-element-container js-form-wrapper form-wrapper"]');

@@ -138,7 +138,7 @@ public function testViewElementEmbed() {
-    $xpath = $this->xpath('//div[@class="views-element-container form-wrapper"]');
+    $xpath = $this->xpath('//div[@class="views-element-container js-form-wrapper form-wrapper"]');

I think that we should remove the non-functional classes from the tests, what do you think?

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes

Adding suggested commit message.

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs manual testing

We had this discussion in other issues, we shouldn't be updating the tests here and instead improve the tests in followup issues.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/system/templates/container.html.twig
@@ -17,4 +17,9 @@
-<div{{ attributes.addClass(has_parent ? 'form-wrapper') }}>{{ children }}</div>
+{%
+  set classes = [
+    has_parent ? 'js-form-wrapper',
+  ]
+%}
+<div{{ attributes.addClass(classes) }}>{{ children }}</div>

+++ b/core/modules/system/templates/fieldset.html.twig
@@ -21,7 +21,13 @@
-<fieldset{{ attributes.addClass('form-item', 'form-wrapper') }}>
+{%
+  set classes = [
+    'form-item',
+    'js-form-wrapper',
+  ]
+%}
+<fieldset{{ attributes.addClass(classes) }}>

Shouldn't we by having both form-wrapper and js-form-wrapper? Like we did in core/lib/Drupal/Core/Render/Element/Details.php?

star-szr’s picture

star-szr
he/him
Primary language English
commented

We don't add those classes to core markup, just Classy. When we don't have a choice (added via modules/JS), we add both.

Edited to use the right word for modules :)

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community

That makes sense for now, hopefully we can move where we are adding the classes in the future.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

git ac https://www.drupal.org/files/issues/2473955-form-wrapper-split.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  6074  100  6074    0     0   4413      0  0:00:01  0:00:01 --:--:-- 17812
error: patch failed: core/misc/states.js:509
error: core/misc/states.js: patch does not apply
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I'm not convinced by #6. What about

.locale-translate-filter-form .form-wrapper {
  margin-bottom:0;
}

in locale.admin.css?

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new prefix_form_wrapper-2473955-10.patch5.94 KB

Thanks, rerolled. I will try to catch @alexpott in IRC to discuss further :)

star-szr’s picture

star-szr
he/him
Primary language English
commented
StatusFileSize
new prefix_form_wrapper-2473955-11.patch6 KB
new interdiff.txt847 bytes

There are quite a few things in core modules using this form-wrapper class so it does seem a bit rash to rip it out.

We can create a followup to discuss the CSS/form-wrapper class further, probably most of the styling could be moved to Classy at which point we could also remove the form-wrapper classes from the core templates. I don't have time right now to create the followup but here's a new patch.

As @alexpott pointed out, the most important thing is getting the js- classes in, not getting the non-js- classes out.

davidhernandez queued 11: prefix_form_wrapper-2473955-11.patch for re-testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

This looks good to me. Covers all cases where I can find form-wrapper. I sent it for retesting to make sure it is still ok.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 9a7a4c2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 9a7a4c2 on 8.0.x 
    Issue #2473955 by Cottser: Prefix form-wrapper classes with js-

Status: Fixed » Closed (fixed)

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