Issue #2473949 by mortendk, rteijeiro, rachel_norfolk, Cottser, LewisNyman: Prefix form-type-* classes with js-

Task

Prefix form-type-* 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

  1. Navigate to admin/config/people/accounts
  2. Confirm that the vertical tabs are functional

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
#5 2473949-5.patch4.25 KBstar-szr
#1 interdiff-2473949-form-type-.txt3.99 KBstar-szr
#1 2473949-form-type--additional.patch5.91 KBstar-szr
#1 2473949-form-type--split.patch4.25 KBstar-szr

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2473949-form-type--split.patch4.25 KB
new 2473949-form-type--additional.patch5.91 KB
new interdiff-2473949-form-type-.txt3.99 KB

Initial patch split from the parent, some additional changes from grepping, and interdiff between the two.

The last submitted patch, 1: 2473949-form-type--split.patch, failed testing.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing

ol>

  • +++ b/core/modules/locale/config/optional/tour.tour.locale.yml
@@ -55,7 +55,7 @@ tips:
-      data-class: form-type-textarea
+      data-class: js-form-type-textarea

    This class is used for styling in Bartik and I can't find where it is used in javascript. We should not convert it to a js class 

  • +++ b/core/modules/options/src/Tests/OptionsWidgetsTest.php
@@ -477,8 +477,8 @@ function testEmptyValue() {
+    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//input[@value=:value]', array(':id' => 'edit-card-1', ':class' => 'form-item js-form-type-radio form-type-radio form-item-card-1', ':value' => '_none')), 'A test radio button has a "None" choice.');
+    $this->assertTrue($this->xpath('//div[@id=:id]//div[@class=:class]//label[@for=:for and text()=:label]', array(':id' => 'edit-card-1', ':class' => 'form-item js-form-type-radio form-type-radio form-item-card-1', ':for' => 'edit-card-1-none', ':label' => 'N/A')), 'A test radio button has a "N/A" choice.');

    I think we should only test for the functional classes

    • star-szr’s picture

    star-szr
    he/him
    Primary language English
    commented

    Thanks for the review. tour.module is where the class is used for JavaScript!

    And to be clear:

    • This is not converting form-type-* classes one by one.
    • Bartik will still get the non-JS form-type-* classes because it extends Classy.

    As for point 2, see #2474107: Make OptionsWidgetsTest::testEmptyValue() care less about markup :)

    star-szr’s picture

    star-szr
    he/him
    Primary language English
    commented
    Status: Needs work » Needs review
    StatusFileSize
    new 2473949-5.patch4.25 KB

    This just rolls back the changes to OptionsWidgetsTest now that #2474107: Make OptionsWidgetsTest::testEmptyValue() care less about markup is in. Also this needs commit credits added for the folks that worked on the original, now meta issue.

    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

    Ahhh ok, that wasn't setting the class, that was using the class. RTBC!

    lewisnyman’s picture

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

    Ahem...

    alexpott’s picture

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

    Committed b48bfcb and pushed to 8.0.x. Thanks!

    • alexpott committed 481ed6c on 8.0.x 
      Issue #2473949 by mortendk, rteijeiro, rachel_norfolk, Cottser,...

    Status: Fixed » Closed (fixed)

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