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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Status: Active » Needs review
FileSize
5.93 KB

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

LewisNyman’s picture

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

Issue summary: View changes

Adding suggested commit message.

LewisNyman’s picture

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

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

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

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

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

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

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.94 KB

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

star-szr’s picture

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’s picture

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

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

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.