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

Files: 
CommentFileSizeAuthor
#5 2473949-5.patch4.25 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,017 pass(es). View

Comments

Cottser’s picture

Status: Active » Needs review
FileSize
4.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,635 pass(es), 2 fail(s), and 0 exception(s). View
5.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,623 pass(es). View
3.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

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

  • Cottser’s picture

    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 :)

    Cottser’s picture

    Status: Needs work » Needs review
    FileSize
    4.25 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,017 pass(es). View

    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.

    Cottser’s picture

    Issue summary: View changes

    Adding suggested commit message.

    LewisNyman’s picture

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

    LewisNyman’s picture

    Status: Needs review » Reviewed & tested by the community

    Ahem...

    alexpott’s picture

    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.