[Meta] Olivero JavaScript should be selecting [data-drupal-selector] attributes where possible

we prefer using data- attribute for JS related selection, that way themers are free to do whatever they need with classes without breaking the JS. It came up a few times the past weeks so something must not be clear on core side.

Essentially: js- prefix: better than a "simple" class, dedicated data- attribute = best.

As I see we are adding data attribute for js classes, I have spent some time looking for the classes which actually needs to be prefixed but found either they already have a data attribute or they are used both in CSS as well as js. Can you guys point to what more to be expected in this issue?

Hi,

I am applying patch for adding js prefix class in js files.

Kindly review once.

Hey @Pooja, We have to update the DOM also with the same changes but that is not needed until and unless the class is used only in js, as @nod_ also commented they prefer data- prefix. Please read my above comment as this will not help us updating the prefixes as they are used in js as well as the same classes are used in CSS.

Let me provide you an example if we change .site-branding__inner class it is used in the below template.

/olivero/templates/block/block--system-branding-block.html.twig

<div class="site-branding__inner">
    {% if site_logo %}
      <a href="{{ path('<front>') }}" rel="home" class="site-branding__logo">
        <img src="{{ site_logo }}" alt="{{ 'Home'|t }}" />
      </a>
    {% endif %}
    {% if site_name %}
      <div class="site-branding__text">
        {% if site_name %}
          <div class="site-branding__name">
            <a href="{{ path('<front>') }}" title="{{ 'Home'|t }}" rel="home">{{ site_name }}</a>
          </div>
        {% endif %}
      </div>
    {% endif %}
  </div>

Now if you are updating in js DOM also needs to be updated and the same class we are using here is CSS line number 79, it will break if you update the class in DOM.
/olivero/css/dist/components/header-site-branding.css

My concern is the same in the above comment:

I have spent some time looking for the classes which actually needs to be prefixed but found either they already have a data attribute or they are used both in CSS as well as js

Yes you are right, but can I provide different css file name for js file.

Let's wait for maintainers comment, I'll try to connect via slack with them.

Okay

Thanks

Welcome @mherchel, Let's me scan through all the js specific attribute and add prefixes.

Hi,

I am applying patch for this issue.

Kindly review the patch once.

Thanks.

I have scanned all the classes and working on a patch now. Moving all the required classes to js- prefix.

Hello @mherchel, I have scanned all the js files and applied the prefix in the patch. Please review.

here is the approach I have followed and made changes

<strong>
js/comments.es6.js
js/comments.js
</strong>

show-hide-wrapper changed to .js-show-hide-wrapper

<strong>Debug.js No change</strong>
.site-branding__inner (header-site-branding.css)
.site-branding.css (site-branding__logo)
.site-branding__name (site-branding__logo)
.olivero-debug(debug.css)
.is-always-mobile-nav(feature-bottom.css)

<strong>
js/messages.es6.js
js/messages.js
</strong>

.messages__container(message.css)
.messages__button
.messages__close
.visually-hidden (Used in multiple css files off-canvas-base.css)
.messages-processed Changed to .js-messages-processed 

All other files either have the js prefix or do exist in css to which the change is not required.

Please review this one as the above is not related and uploaded by mistake.

Removing new line change in this patch.

Hi,

thedrupalkid you missed some where to include js prefix class.

I am applying updated patch.

Kindly review once.

Let me know for any correction.

Thanks.

@Pooja: Thanks for the patch, as Mike has suggested testing changes before creating a patch will be beneficial and the additional changes will break the js functionality as we are using it in CSS as well as in DOM.
Changes to this:

const overlay = document.querySelector('.js-overlay');
".js-overlay" does not exist.

Also used on page.template.twig which is very generic and break everything:

Also changing this

-    if (isTabsMobileLayout() && !activeTab.matches('.tabs__tab:first-child')) {

will affect this:

<li{{ attributes.addClass('tabs__tab', is_active ? 'is-active') }}>

So these additional changes are not required.

Okay Thanks @thedrupalkid for your suggestions.

Hi,

I am creating a patch for this issue on 9.1.x-dev drupal version.

Please review the patch.

Thanks.

updated patch.

I have set a data attribute instead of class 'show-hide-wrapper'.

Hi,

Creating a patch to including changes to JS selectors.

Kindly review the patch.

Let me know if any suggestions.

Thanks.

messageWrapper.setAttribute('data-messages-list__item messages messages--', "messages-list__item messages messages--".concat(type, " messages-processed"));

which attribute is data-messages-list__item messages messages--? I'm not sure how to replace it, but it is seems wrong like that.

Hi. I think I understand what needs to happen here. How would you feel about using data-olivero-* attributes for these selectors? I think it would be beneficial to set a convention whereby not only do we use data-* attribute selectors (a direction which I very much support), but we go one step further and scope these selectors to be Olivero-specific by using an olivero- prefix (hence data-olivero-*). This is not unlike Drupal’s own data-drupal-* attribute selectors (data-drupal-nav-tabs, for instance).

I’ve created a very contrived patch that accomplishes my proposed solution for the selectors in tabs.es6.js as well as associated template files to get opinions on this direction. Please bear in mind this is by no means an exhaustive patch; it is only meant to illustrate how this idea would look before spending any significant amount of time on this.

I look forward to hearing your feedback. Thanks!

Re-rolled patch in #43.

How would you feel about using data-olivero-* attributes for these selectors?

I think this could cause confusion down the line for themes that use Olivero as a base theme, or use a contrib theme that uses Olivero as a base theme. I'm thinking particularly of how extensively classy is used by custom and contrib themes as a base theme.

Also, if this naming convention was used for attributes in a JS theme function (which can be overridden by a theme), this could break functionality. While that could technically happen with any data- attributes, having the theme name in the attribute might give the mistaken impression that they are not needed or should be changed to include the theme's name. e.g. data-awesome-theme-*

If using an additional identifier for data- attributes is needed, I would prefer a more generically named one. Is there a reason data-drupal-* should not be used? I'm not sure if that convention is only supposed to be used for core modules.

@mherchel would it perhaps be better if this issue became a meta issue and separate sub-issues were created for the changes to each JS file (and associate twig template)? It might make it easier to review changes, test, and spread out the workload.

I think this could cause confusion down the line for themes that use Olivero as a base theme, or use a contrib theme that uses Olivero as a base theme. I'm thinking particularly of how extensively classy is used by custom and contrib themes as a base theme.

Good point, @PCate.

Is there a reason data-drupal-* should not be used? I'm not sure if that convention is only supposed to be used for core modules.

This is why I was reluctant to suggest data-drupal-*, but given the points you stated above, I’m inclined to suggest we move forward without scoping (i.e. data-*).

---

Creating separate tasks for each JS file in the theme makes sense, especially from a reviewing/testing perspective. Happy to contribute to this issue further once we have a definitive way forward!

Is it just #3212975: Olivero: Normalize JavaScript selectors in messages.es6.js left to do here? If so should we switch out the issues on #3177296: [META] Make Olivero stable and close this meta?