Problem/Motivation

Create hasClass() methods on Attribute object for testing if class exists.

It would be useful to be able to do functionality based on applied classes e.g replace classes in Twig template.

Proposed resolution

Add methods to the Attribute object for testing if class exists.

@mortendk proposed method name hasClass which would return boolean based on existance of class.

Example syntax within Twig

{% if attributes.hasClass('foo') %}
  {{ attributes.addClass('bar') }}
{% endif %}
{% if attributes.hasClass('field-label-inline') %}
    ...print markup like this.
{% else %}
    ...print it like this.
{% endif %}

Remaining tasks

Add a change record - Novice task

User interface changes

n/a

API changes

API addition to test if class exists on Attribute object.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jjcarrion’s picture

Issue summary: View changes
kasn’s picture

Assigned: Unassigned » kasn
Status: Active » Needs review
FileSize
1.86 KB

add hasClass Method as proposed

lauriii’s picture

Test patch to prove tests are working

Status: Needs review » Needs work

The last submitted patch, 3: drupal-attribute-new-method-has-class-2347511-1-tests.patch, failed testing.

star-szr’s picture

In the example in the summary I don't understand the use case, I don't see the harm in removing the if in that case. Can we get more information in the issue summary please?

davidhernandez’s picture

Issue summary: View changes

The use case is largely the same as the Jquery one. http://api.jquery.com/hasclass/ . It is just a bit simpler for themers to check for a class than check for some more complicated things.

lauriii’s picture

Assigned: kasn » Unassigned
Status: Needs work » Needs review
davidhernandez’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
@@ -141,6 +141,19 @@ public function testRemoveClasses() {
diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml

diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php

diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
old mode 100644
new mode 100755

This permission change shouldn't be in the patch, correct?

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

fixed

davidhernandez’s picture

I moved hasClass down to keep addClass and removeClass together. Updated all the comments. Removed the extra class name from the test. It wasn't necessary.

The last submitted patch, 10: add_method_to_test_if-2347511-10-testonly.patch, failed testing.

drifter’s picture

Status: Needs review » Reviewed & tested by the community

Tested this manually in a twig template (both having a class and not having a class), works well. The method and test is straightforward, parameters are documented, looks good to me.

davidhernandez’s picture

Issue tags: +Needs change record

Being an API addition, it probably could use a change record.

davidhernandez’s picture

Issue summary: View changes
Issue tags: +Novice

Just adding the novice tag to indicate the change record is a novice task.

mirom’s picture

Assigned: Unassigned » mirom
lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Putting back to Needs Review for the Change record

mirom’s picture

lauriii’s picture

Assigned: mirom » Unassigned
Status: Needs review » Reviewed & tested by the community

The change record looks good! Lets go with this!

lauriii’s picture

Issue tags: -Needs change record
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
diff --git a/core/tests/Drupal/Tests/Core/Template/AttributeTest.php b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
index 49da086..6413a3b 100644
--- a/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
+++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
@@ -145,6 +145,10 @@ public function testRemoveClasses() {
    * @covers ::hasClass()
    */
   public function testHasClass() {
+    // Test an attribute without any classes.
+    $attribute = new Attribute();
+    $this->assertFalse($attribute->hasClass('a-class-nowhere-to-be-found'));
+
     // Add a class to check for.
     $classes = array('we-totally-have-this-class');
     $attribute = new Attribute(array('class' => $classes));

This fails and it shouldn't.

$ phpunit --filter AttributeTest
PHPUnit 4.1.4 by Sebastian Bergmann.

Configuration read from /Volumes/devdisk/dev/drupal/core/phpunit.xml.dist

......E..............

Time: 2.1 seconds, Memory: 96.00Mb

There was 1 error:

1) Drupal\Tests\Core\Template\AttributeTest::testHasClass
Undefined index: class

/Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Template/Attribute.php:200
/Volumes/devdisk/dev/drupal/core/tests/Drupal/Tests/Core/Template/AttributeTest.php:150

FAILURES!
Tests: 21, Assertions: 55, Errors: 1.
davidhernandez’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
1.66 KB

Fixing #20

cilefen’s picture

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -189,6 +189,22 @@ public function removeClass() {
    +   *   CSS class to check for.
    

    "The" CSS class...

  2. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -189,6 +189,22 @@ public function removeClass() {
    +    } else {
    +      return FALSE;
    +    }
    

    Else should be on a new line.

cilefen’s picture

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -189,6 +189,22 @@ public function removeClass() {
+   * @return bool
+   */

The @return is missing its comment.

sarav.din33’s picture

Status: Needs review » Reviewed & tested by the community

Hi davidhernandez,

Patch #20 works fine.

Test case:

In my bartik theme's node.html.twig file, i added the below snippet,

{% if 'node' in attributes.class.value %}
  <div class="custom-node-container">
  
  </div>
{% endif %}
{% if 'contextual-region' in attributes.class.value %}
  <div class="contextual-region-container">
  
  </div>
{% endif %}

Which means check whether the class named as 'node' exist in the current content or not. Because i wrote the code in node.html.twig so we can test it in content page only. If 'node' class is exists i just print the "custom-node-container" div in it. Likewise for 'contextual-region' class.

Once you did the changes in twig files you need to clear the cache then only it will reflect in your site. After clear the cache, i can able to see the those two div containters in the page.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

Marking needs work for the code style issues.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
805 bytes
1.83 KB

Fixed code style issues.

cilefen’s picture

Status: Needs review » Reviewed & tested by the community

Based on a manual test verifying this works, and that the code style looks correct, this is RTBC.

cilefen’s picture

@sarav.din33 It looks as though you did not test this method.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a prioritized change (the method added here helps write cleaner templates) as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? and it's benefits outweigh any disruption. Committed 1d8f7c5 and pushed to 8.0.x. Thanks!

  • alexpott committed 1d8f7c5 on 8.0.x
    Issue #2347511 by davidhernandez, lauriii, rpayanm, kasn: Add method to...

Status: Fixed » Needs work

The last submitted patch, 26: 2347511-26.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Fixed
sarav.din33’s picture

@cilefen: Noted will make sure i do a code check along with the review.

Status: Fixed » Closed (fixed)

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