[meta] Add in js- prefixed classes for separation of JS & CSS functionality

Re-rolled and tested. Everything seems ok so mortendk didn't break anything :P

Form BEFORE

Formn AFTER

Where we are inserting the js-**** class, there are a couple of examples of where we remove some **** clsses that *might* be expected by themers. Some themers might be tempted to then use the js-**** classes to apply styles and that's not what we want.

An example would be

+++ b/core/modules/field_ui/field_ui.js
index 94feeff..3ceb0ae 100644
--- a/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
+++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php

@@ -307,7 +307,7 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, arr
           '#title_display' => 'invisible',
           '#options' => array_combine($regions, $regions),
           '#empty_value' => '',
-          '#attributes' => array('class' => array('field-parent')),
+          '#attributes' => array('class' => array('js-field-parent')),
           '#parents' => array('fields', $field_name, 'parent'),
         ),

While it's a pain to add classes rather than remove them, I don't think we have a choice here.

Could always bite the bullet and go for data attributes...

I'm going to have a go at these data attibute things - might need people leaning over my shoulder checking...

Today and probably tomorrow at #dclondon.

So after much whiteboard writing and crossing out with Morten, I've just added back in any "plain" (think non-js-***) classes back into the code.

We can now look to take all the js-classes and make them into data-* attributes next - once we decide on a naming scheme for them!

As a note of what we have found out so far about data-* attributes: (with help from JanneKalliola)

• it's good to make the attibutes descriptive! data-js="something" isn't really the way to go;
• we can store useful information in the data that could be used to simplify some js - say something like data-js-submit="Are you sure remove comment 4?" could pass that text on for jQuery to display in a confirmation;
• *if* the html spec allows data-js-* attributes without a value, then we can use them.

Using data-js-* attribute names is fine. It's similar to how other frameworks, like Angular, structure functional attributes: https://docs.angularjs.org/guide/directive

Right - I'm going to have a go at it then!

We can do this in two steps, having classes prefixed by js- will allow CSS folks to figure out their dependencies on those classes without messing up the js. That looks done no?

Once we're there we can go back to the JS and see what to do on the JS side. For the data attribute that's being discussed in #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests too. I think we should have a drupal in the name somewhere to avoid conflicts with other frameworks. In the issue data-html-id is proposed (I would prefer data-drupal-id but that's a detail).

So, for my own memory as much as anything on a Sunday after a Drupalcamp Saturday night:

1. yes, we can use data-* attributes and they should be of the form data-drupal-*;
2. yes, we can have valueless data-drupal-* attributes;
3. we should be obsessive to separate js and css selectors - so js selects against data-drupal-* and css does not;
4. when we add a class via js, we prefix it js-* so we know it was set by js.

Does that sound good and should it be written down somewhere more useful than the middle of an issue?

RE: #29, 3 - do we need to ever add a class via JS? Can we not set a data attribute? If the use case is to add a class that is used for style, that sohuldn't need a js- prefix.

#31 Adding classes using javascript is still possible, and most third party plugins will still use it, I guess this issue is more of what is outputted by the server.

#29 I think it is best to update the issue summary to reflect your proposal

1. +++ b/core/misc/states.js
   @@ -509,7 +509,7 @@
   +        .closest('.js-form-item, .js-form-submit, .js-form-wrapper').toggleClass('form-disabled', e.value)
   

   Should this be: js-form-disabled ?

2. +++ b/core/modules/system/templates/container.html.twig
   @@ -17,4 +17,4 @@
   +<div{{ attributes.addClass(has_parent ? 'js-form-wrapper') }}>{{ children }}</div>
   

   Should this contain form-wrapper class too?

3. +++ b/core/modules/system/templates/datetime-wrapper.html.twig
   @@ -17,7 +17,7 @@
   -    required ? 'form-required',
   

   Should we keep this?

4. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -21,10 +21,10 @@
   +<fieldset{{ attributes.addClass('js-form-item', 'js-form-wrapper') }}>
   

   Should we keep form-item and form-wrapper classes?

5. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -21,10 +21,10 @@
          required ? 'form-required',
   

   Should we remove this one then?

6. +++ b/core/modules/system/templates/form-element-label.html.twig
   @@ -16,9 +16,8 @@
   -    required ? 'form-required',
   

   Should we keep this one?

1. +++ b/core/themes/classy/templates/form/fieldset.html.twig
   @@ -21,10 +21,11 @@
          required ? 'form-required',
   

   So then this should be removed too.

2. +++ b/core/themes/classy/templates/form/form-element-label.html.twig
   @@ -18,6 +18,7 @@
        required ? 'form-required',
   

   And this one should be removed as well.

I've had a look through the code and it looks good to me. I'm not an expert on the test classes so would like someone to check those are still valid.

The forms js continues to run without error on the fields I have tested. The js-* classes all appear to be in place. Any js only uses js-* classes as selectors, as desired.

The form appearance has not been changed by this patch.

Back to RTBC I guess.

A few points/questions, this is looking very good overall though!

1. +++ b/core/includes/theme.inc
   @@ -492,8 +492,8 @@ function template_preprocess_datetime_wrapper(&$variables) {
   +  // For required datetime fields a 'form-required' & 'js-form-required' class
   +  // is appended to the label attributes.
   

   Hate to say it but: grammar! "…fields 'form-required' & 'js-form-required' classes are appended…"

2. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -21,24 +21,23 @@
   -      'fieldset-legend',
   -      required ? 'form-required',
   +      required ? 'js-form-required'
   

   Trailing comma here please to match our array coding standards: https://www.drupal.org/coding-standards#array.

3. +++ b/core/modules/system/templates/form-element-label.html.twig
   @@ -16,9 +16,8 @@
   -    required ? 'form-required',
   +    required ? 'js-form-required',
   
   +++ b/core/themes/classy/templates/form/form-element-label.html.twig
   @@ -18,6 +18,7 @@
   +    required ? 'js-form-required',
        required ? 'form-required',
   

   I gather from #39 #40 why the core templates remove form-required but the classy templates do not. But just want to mention styling for form-required still exists in system.theme.css and make sure we're cool with that :) I am probably missing a fundamental banana concept here.

4. +++ b/core/themes/classy/templates/form/container.html.twig
   @@ -17,4 +17,4 @@
   -<div{{ attributes.addClass(has_parent ? 'form-wrapper') }}>{{ children }}</div>
   +<div{{ attributes.addClass(has_parent ? 'js-form-wrapper form-wrapper') }}>{{ children }}</div>
   

   Short a space after the first comma :)

   +++ b/core/themes/classy/templates/form/fieldset.html.twig
   @@ -21,10 +21,11 @@
   -<fieldset{{ attributes.addClass('form-item', 'form-wrapper') }}>
   

   Should we split both Classy's fieldset and container classes out into a set classes block? We usually do when it's more than one or two or if there's complex logic, I think these both fit that criteria.

Cool, interdiff doesn't make sense but I eyeballed the changes and they all look good but the grammar part seems to have stayed the same, my evil eyes are not detecting any changes to those lines :(

Looking better, thanks @mortendk, add two characters and I'm happy :)

+++ b/core/includes/theme.inc
@@ -492,8 +492,8 @@
+  // For required datetime fields 'form-required' & 'js-form-required' class

class = classes :)

More importantly, were the changes to indentation.html.twig and theme_indentation() removed on purpose because of #2449445: Add "indentation" class back to indentation theme hook, use it for styling? I noticed earlier patches had those changes and it may conflict but I think we should add that back here because importantly the patch here adds both indentation and js-indentation to the theme function and template file, one for CSS, one for JS.

I think a change record would be great here. I noticed the change record for #2426595: Rename indentation class to js-indentation didn't get published so let's keep it that way and work on a bigger change record to encompass all of these changes.

Also we need a beta evaluation in the issue summary and the issue summary needs a bit of love. And the banana tag is like the batman version :)

Dreditor has a beta evaluation template button, I can help with that later though if you ping on IRC.

On the indentation it's a bit complicated IMO because we also have a theme function which is the default output unless we add a theme function override in Classy. Right now there is a mismatch.

For practical purposes I think we need to do one of these:

1. Update the default indentation.html.twig so that it has both indentation and js-indentation.
2. Add indentation.html.twig to Classy which has both classes and then #2449445: Add "indentation" class back to indentation theme hook, use it for styling can go in as-is, no changes needed. Also add a theme function override to Classy with the additional indentation class and remove indentation from theme_indentation(). That way non-classy would never get the indentation class, and Classy would, consistently. Classy doesn't have a .theme now but we could add one?

Edited to make the second option make sense for both Classy and core's default markup.

1. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
   @@ -307,7 +307,7 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, arr
   -          '#attributes' => array('class' => array('field-parent')),
   +          '#attributes' => array('class' => array('js-field-parent', 'field-parent')),
   
   @@ -486,7 +486,7 @@ protected function buildExtraFieldRow($field_id, $extra_field) {
   -          '#attributes' => array('class' => array('field-parent')),
   +          '#attributes' => array('class' => array('js-field-parent', 'field-parent')),
   

   There is a line in field_ui.js that needs to be changed to actually use the js- class:

   $row.find('select.field-parent').val('');

   There is also this line in field_ui.inc that should be updated:

   '#attributes' => array('class' => array('field-parent')),

2. Some code that needs to be updated for js-form-item:

   core/misc/machine-name.js:
   var $wrapper = $target.closest('.form-item');

   core/modules/filter/filter.filter_html.admin.js:
   that.$allowedHTMLDescription = that.$allowedHTMLFormItem.closest('.form-item').find('.description');

   Should we update language-negotiation-configure-form.html.twig?:

     {%
       set language_classes = [
         'form-item',
         'table-language-group',
         'table-' ~ language_type.type ~ '-wrapper',
       ]
     %}

   core/modules/locale/locale.admin.js:
   $rowToMark.find('td:first-child .form-item').append(marker);

   field-multiple-value-form.html.twig?:
   <div class="form-item">

   form-element.html.twig?:

   {%
     set classes = [
       'form-item',
       'form-type-' ~ type|clean_class,
       'form-item-' ~ name|clean_class,
       title_display not in ['after', 'before'] ? 'form-no-label',
       disabled == 'disabled' ? 'form-disabled',
     ]
   %}

   I'm stopping here for form-item. Search yourself for form-item and you'll find a lot of things that probably need to be updated throughout core.

3. +++ b/core/modules/system/templates/fieldset.html.twig
   @@ -21,24 +21,29 @@
   -      'fieldset-legend',
   ...
   -  <div class="fieldset-wrapper">
   +  <div>
   ...
   -      <span class="field-prefix">{{ prefix }}</span>
   +      <span>{{ prefix }}</span>
   ...
   -      <span class="field-suffix">{{ suffix }}</span>
   +      <span>{{ suffix }}</span>
   
   +++ b/core/modules/system/templates/form-element-label.html.twig
   @@ -16,9 +16,8 @@
   -    title_display == 'after' ? 'option',
   

   I know you're having fun but IMO these changes are out of scope :)

I only grepped field-parent and form-item with the patch applied, this needs to be gone through very carefully I think.

Also this part from the issue summary is not clear to me, what's the difference in the two lists? "classes added & used by js" and "selectors changed in js"?

Not sure if we have a higher level meta yet, so adding in this old issue about using data attributes so it doesn't get lost.

Not saying we need to change direction here :)

Re-rolled!

With the patch size being that much now, would it make any sense to break this down and make it a meta? Over in #2407565: Consensus Banana Phase 1, cleanup I just identified that form-managed-file should be js- prefixed as well.

Maybe we can do some logical grouping so we don't have merge conflicts and such, I'm not necessarily suggesting 1 class per issue.

I'm going for it (splitting this up), I think this needs a push forward and I think that is the best way. This patch as is can be maintained as is but it's going to be very hard IMO to get it reviewed and tested.

Will update here once done :)

Split things big, all misc/ in one patch, all themes in one patch (they don't have much CSS) and at most one issue by module. One issue per file is horrible to manage, did it a couple of times for jshint/eslint and clean-up, won't do it again.

I was thinking of doing one issue per class in the issue summary, I figured that would be easier to test. I'm also not sure if this one could actually be split by module without leaving things in a broken state.

First a reroll.

Almost done, I'm going to make the sub-issues tomorrow, update this issue's summary, and post an interdiff of the changes that didn't make it into any of the sub-issues (really just the out of scope type stuff I pointed out in #66.3).

Updating issue summary and converting to a meta. All child issues have a patch. Attaching the interdiff here of the changes that didn't make the cut into the child issues.

Every other change from the latest patch here is in one of the child issues, I combined all the child issues into a git branch and diffed against the latest patch here to produce the attached interdiff.

I talked about this with @catch today... nothing conclusive :(

@alexpott: Any feelings about https://www.drupal.org/node/2431671?
@catch: I liked it until I saw the patch.
Looks like it's duplicating some class so they have with and without the prefix?
@alexpott: Yep
Because we still have styling that relies on the class too
I'm torn
because if you were developing your own cms the duplication would be stupid
but telling themers this is needed for javascript will save a lot of current headaches
@catch: So the issue is, if we ended up not needing it for js, then we might remove the class altogether, then the styling wouldn't work.
So having both stops that.
But... nothing stops someone styling based on a non-duplicated js-prefixed class.
Then you have the same problem.
So I'd say either duplicate everything, or don't duplicate at all.
@alexpott: Do you mind if I post some of this on the meta?
@catch: Fine with me.

Would going all the way and using data attributes for the JS functionality help? We saw this as a stepping stone but if it's too contentious…

Small comment:

But... nothing stops someone styling based on a non-duplicated js-prefixed class.
Then you have the same problem.
So I'd say either duplicate everything, or don't duplicate at all.

Of course we can't stop someone styling with a js-prefixed class :), but: Classy always duplicates. So is the last line proposing that non-Classy duplicate the classes as well?

RE #88, there are a couple mitigating factors, though the big issue is whether we'll get them done in time for release. Most of the non-js classes would get removed from core templates, so the duplication would only be in Classy. Another factor is eventually moving these to data attributes, so we can get rid of the classes.

Right now splitting them off would let us keep moving forward with the whole declassification without having to rework all the JS.

Cottser and I were just talking about this. We could skip straight to using data attributes, if that is more palatable. We'll need to evaluate how much work that is. We would also likely expand this, as the current issues are just tackling the form classes.

Even if you were developing your own CMS, the duplication might well be useful. I do something similar when I'm creating markup for our themers (which they lovingly call "unstyled garbage"). Any class that is added for the benefit of people writing JS gets a js- prefix. Any class added for the CSS folks gets no prefix. It makes changing markup later so much easier.

Moving straight to data attributes would be a fine idea if there were time, but using classes isn't yucky or anything. It's still a nice improvement.

@ksenzee - yep I agree - I'm +1 on the approach of duplicating the classes. I'll try to get consensus with the other maintainers.

Do we have consensus on the approach?

I think in the three issues remaining we are going with the classy approach, to keep it simple and get the prefixed classes in.

<div class="foo"> -> <div class ="js-foo foo">

Yeah going straight to data was risky so we agreed on doing all the js- prefix first, which will make the move to data attributes easier afterwards.

All done!

This is not done, there are a bunch of scripts still using classes. Like toolbar, tabledrag or blocks

Well, done as far as the child issues we outlined were needed for the Classy changes. I think that was mostly the form items. Are we going to work on others? I'm not sure whether we can do that post release.

We should be able to if we don't remove the old classes

There hasn't been discussion here for 10 years. Is this still relevant to a currently supported version of Drupal.

Is there work here to do?

Wow. This is a blast from the past!

This issue was a stepping stone on the way to data-* attributes we now know and love.

We love them, right? ;-)

So, this work has now been superseded and this issue can be closed. To be honest, it was part of what got us to where we are so, whilst it was never merged, I think I could argue it was “fixed”.

I’ll let a maintainer make that decision, though.