Prefix classes needed for JS functionality with 'js-' [#2877358]

Drupal core prefixes classes needed to provide Javascript funtionality wir 'js-'. Commerce should do so as well to indicate which classes can be removed and which are required. One example in commerce_cart:

Current

commerce_cart.module:

<?php
/**
 * Prepares variables for the cart block element template.
 */
function template_preprocess_commerce_cart_block(&$variables) {
  $variables['attributes']['class'][] = 'cart--cart-block';
}

?>

commerce_cart.js:

var $cart = $context.find('.cart--cart-block');

Should be

commerce_cart.module:

<?php
/**
 * Prepares variables for the cart block element template.
 */
function template_preprocess_commerce_cart_block(&$variables) {
  $variables['attributes']['class'][] = 'js-cart--cart-block';
}

?>

commerce_cart.js:

var $cart = $context.find('.js-cart--cart-block');

Comment	File	Size	Author
#3	prefix_classes_needed-2877358-3.patch	7.26 KB	gauravjeet
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

10 May 2017 at 14:24

Lukas von Blarer created an issue. See original summary.

Comment #2

Lukas von Blarer

he/him

CreditAttribution: Lukas von Blarer commented 10 May 2017 at 14:24

Title:

Prefex classes needed for JS functionality with 'js-'

» Prefix classes needed for JS functionality with 'js-'

Comment #3

gauravjeet CreditAttribution: gauravjeet as a volunteer and at Acro Commerce commented 2 June 2017 at 12:03

Status:

Active

» Needs review

File	Size
prefix_classes_needed-2877358-3.patch	7.26 KB

Have prefixed class names with 'js-'. Only those class names been changed that are being added into Library via #attributes.

https://github.com/drupalcommerce/commerce/pull/743

Comment #4

Thomas Cys

Dutch

CreditAttribution: Thomas Cys as a volunteer commented 2 June 2017 at 07:05

Great work but in my opinion .js-* classes should only be used to denote behaviour.

Comment #5

Lukas von Blarer

he/him

CreditAttribution: Lukas von Blarer commented 25 June 2017 at 15:31

What do you mean by behavior?

The "js-" class prefixing is there to indicate that removing it might break javascript, right?

Comment #6

Thomas Cys

Dutch

CreditAttribution: Thomas Cys as a volunteer commented 25 June 2017 at 19:11

What i mean is that you shouldn't style "js-" prefixed classes.

Comment #7

Lukas von Blarer

he/him

CreditAttribution: Lukas von Blarer commented 3 July 2017 at 23:15

Ok, then we agree that all classes required by javascript should be prefixed?

Comment #8

Thomas Cys

Dutch

CreditAttribution: Thomas Cys as a volunteer commented 4 July 2017 at 19:47

Ofcourse but they shouldn't have css attached to them.

Comment #9

Lukas von Blarer

he/him

CreditAttribution: Lukas von Blarer commented 9 July 2017 at 18:14

Yes, for that separate classes are needed. I guess we leave the existing ones in place and duplicate them with the js- prefix?

Comment #10

Lukas von Blarer

he/him

CreditAttribution: Lukas von Blarer commented 15 July 2017 at 23:39

Is it too late to get this in before a stable release?

Comment #11

bojanz CreditAttribution: bojanz at Centarro commented 16 July 2017 at 10:04

It can still happen.

Comment #12

Lukas von Blarer

he/him

CreditAttribution: Lukas von Blarer commented 8 August 2017 at 09:40

Status:

Needs review

» Needs work

The patch is looking good so far. I encountered a few occurances of the .cart class:

CartBlock.php:

$cart_views[] = [
  '#prefix' => '<div class="cart cart-block">',
  '#suffix' => '</div>',
  '#type' => 'view',
  '#name' => $available_views[$cart_id],
  '#arguments' => [$cart_id],
  '#embed' => TRUE,
];

CartController.php:

$build[$cart_id] = [
  '#prefix' => '<div class="cart cart-form">',
  '#suffix' => '</div>',
  '#type' => 'view',
  '#name' => $cart_views[$cart_id],
  '#arguments' => [$cart_id],
  '#embed' => TRUE,
];

I guess those are not needed by JS, but they are kind of hard to get rid of. Can we solve this differently?