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');
CommentFileSizeAuthor
#3 prefix_classes_needed-2877358-3.patch7.26 KBgauravjeet

Comments

Lukas von Blarer created an issue. See original summary.

luksak’s picture

luksak
he/him
commented
Title: Prefex classes needed for JS functionality with 'js-' » Prefix classes needed for JS functionality with 'js-'
gauravjeet’s picture

gauravjeet commented
Status: Active » Needs review
StatusFileSize
new prefix_classes_needed-2877358-3.patch7.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

thomas cys’s picture

thomas cys
Primary language Dutch
commented

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

luksak’s picture

luksak
he/him
commented

What do you mean by behavior?

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

thomas cys’s picture

thomas cys
Primary language Dutch
commented

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

luksak’s picture

luksak
he/him
commented

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

thomas cys’s picture

thomas cys
Primary language Dutch
commented

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

luksak’s picture

luksak
he/him
commented

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

luksak’s picture

luksak
he/him
commented

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

bojanz’s picture

bojanz commented

It can still happen.

luksak’s picture

luksak
he/him
commented
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?

luksak’s picture

luksak
he/him
commented
Status: Needs work » Needs review

But actually this is a different issue. We should move that HTML to templates. I created an issue for that: #2900616: Move HTML to twig templates

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Version: 8.x-2.x-dev » 3.0.x-dev
Status: Needs review » Needs work

Would someone like to prepare this as MR against 3.0.x or should we close this won't fix due to inactivity since 2017?