Problem/Motivation

We expose many class attributes in templates and other layers (for example preprocess), but it's never clear which classes are needed for JavaScript functionality, leaving themers in the dark if they want to slim down their markup and not break JS.

Proposed resolution

Add CSS classes for JavaScript functionality. Classes will use the original name and be prefixed with .js- and be added to the existing class (in Classy) for separating design and functionality.

Classy

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

Core

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

This is a step along the way to using data attributes, but even if we don't get to data attributes it's still an improvement.

Remaining tasks

See child issues.

User interface changes

None for themes extending Classy. Possible visual changes to themes not extending Classy.

API changes

n/a

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because nothing is broken
Issue priority Not critical because nothing is broken
Unfrozen changes Unfrozen because it mostly just affects templates and JS which is not frozen
Prioritized changes The main goal of this issue is to improve themer experience and arguably it reduces fragility where JavaScript and markup intersect.
Disruption Shouldn't be too disruptive as it is mostly affecting CSS classes that are added to markup. Themes extending Classy will only have classes added. Themes not extending Classy will have classes removed but they can be added back via template overrides.
Files: 
CommentFileSizeAuthor
#87 interdiff-2431671-js-prefix-out-of-scope.txt1.89 KBCottser
#84 2431671-84.patch56.59 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,428 pass(es), 2 fail(s), and 0 exception(s). View

Comments

mortendk’s picture

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
mortendk’s picture

FileSize
12.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,289 pass(es), 6 fail(s), and 0 exception(s). View

first stap at this .form-required

.js-form-required is used for the selectors .find & .toggleclass where its used for js
the original form-required is still used for styling.

classes prefixed as selectors
.js-form-item, .js-form-submit, .js-form-wrapper js-form-required

mortendk’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: form-required-prefixed-js.diff, failed testing.

mortendk’s picture

FileSize
18.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,293 pass(es), 8 fail(s), and 0 exception(s). View

.js-form-wrapper cleaned up

mortendk’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: form-required-prefixed-js-2.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
19.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,232 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 13: form-required-prefixed-js-3.diff, failed testing.

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Status: Needs work » Needs review
FileSize
21.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,291 pass(es), 6 fail(s), and 0 exception(s). View

.js-field-parent prefixed

Status: Needs review » Needs work

The last submitted patch, 16: form-required-prefixed-js-4.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
21.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,293 pass(es). View
mortendk’s picture

Issue summary: View changes
rteijeiro’s picture

Issue summary: View changes
FileSize
521.98 KB
601.01 KB
16.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,340 pass(es). View

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

Form BEFORE

Formn AFTER

mortendk’s picture

Issue tags: +dclondon
rachel_norfolk’s picture

Status: Needs review » Needs work

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...

rachel_norfolk’s picture

Assigned: Unassigned » rachel_norfolk

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.

rachel_norfolk’s picture

Assigned: rachel_norfolk » Unassigned
FileSize
17.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch form-required-prefixed-js-2431671-24.patch. Unable to apply patch. See the log in the details link for more information. View
3.88 KB

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!

rachel_norfolk’s picture

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.
jessebeach’s picture

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

rachel_norfolk’s picture

Assigned: Unassigned » rachel_norfolk

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

nod_’s picture

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).

rachel_norfolk’s picture

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?

mortendk’s picture

yes it should and make sense :)
... but im to hungover right now to figure out where it should be done

davidhernandez’s picture

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.

attiks’s picture

#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

mortendk’s picture

+1 for getting the css - js seperation done now then we can figure out the data-attributes later. The seperation is the important thing here.

mortendk’s picture

Status: Needs work » Needs review
Issue tags: +banana-blocker
FileSize
16.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch form-required-prefixed-js-2431671-34.patch. Unable to apply patch. See the log in the details link for more information. View

I would (humbly) suggest that we move the data- attribute discussion to a follow gets the js- & css seperation done now.
Then we can move on and clean up the css & make the banana seperation work, which this is a blocker for (a banana blocker)

Im getting very nervous that well be hanging in "make perfect" solutions, so we never get to the most important thing in Drupal 8 (...) Which is separate & clean up so the CSS dont screw with the JS.

im reuploading patch #18 for review renamed to #34

Status: Needs review » Needs work

The last submitted patch, 34: form-required-prefixed-js-2431671-34.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
15.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,920 pass(es), 67 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 36: form-required-prefixed-js-2431671-36.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
19.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,988 pass(es). View
rteijeiro’s picture

  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?

mortendk’s picture

1. form-disabled - nope form disabled is just styling?
Afaik we dont have a policy that says that classes that gets added by js needs to be prefixed ?

2. nope it should only test on the.js-form-wrapper, if it test on form-wrapper then it would break if a theme did .epic-form-wrapper__something--clever

3. .form-required isp ure styling as well, no functionality

4. nope everything used by javascript we should make with a prefix, so themer dont break something by accident. so we dont wanna keep the non prefixed in core. we wanna make it clear that its "only" visual class

5 + 6 yes we should both of em, to make sure we dont have created any dependencies on em :)

mortendk’s picture

Issue tags: -dclondon
FileSize
19.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,060 pass(es). View
542 bytes
rteijeiro’s picture

  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.

mortendk’s picture

1+2 nope they are both in classy :) so no cleanup there (yet)

rachel_norfolk’s picture

Assigned: rachel_norfolk » Unassigned
FileSize
446.37 KB

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.

screenshot of form

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC I guess.

The last submitted patch, 24: form-required-prefixed-js-2431671-24.patch, failed testing.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

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.

mortendk’s picture

Status: Needs work » Needs review
FileSize
19.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,120 pass(es). View
542 bytes

@cottser
3. yes .form-required is styling that we dont care about for core templates - but do in classy. When we get there in bananan #3 aka the-big-css-reconstruction-road-to-nirvana well take care of that part in short yes were very cool with that (+ system.css have so much stuff in it tthat it needs a proper cleaning to)

fixed the others issues and split ud and set a classes for the templates with multiple classes in, its easier to read & play with :)

new patch & interdiff is uploaded & i even fixed the grammar ;) ... /me thinx

Cottser’s picture

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 :(

mortendk’s picture

Issue tags: +Needs change record
FileSize
19.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,124 pass(es). View
3.11 KB

evil eye cottser do it again

// For required datetime fields 'form-required' & 'js-form-required' class
  // are appended to the label attributes.
  if (!empty($element['#required'])) {
    $variables['required'] = TRUE;
  }
  

patch & interdiff uploaded

I guess we also need a change record to this ?

Cottser’s picture

Status: Needs review » Needs work
Issue tags: -bananana +banana, +Needs issue summary update, +Needs beta evaluation

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 :)

mortendk’s picture

Status: Needs work » Needs review
Issue tags: -banana-blocker
FileSize
19.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,121 pass(es). View

.js-indentation & .indentationare fixed in other issues so no need to have it in here as well. just checked it looks right.

yes classes as there's more than one.

+1 lets do a big class update for js form's, else ppl would be confused.

So i have no idea of what should be in a beta evalution? need a bit of pointers there

Cottser’s picture

Status: Needs review » Needs work

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.

mortendk’s picture

Issue summary: View changes

@cottset updated the patch #2449445: Add "indentation" class back to indentation theme hook, use it for styling so we keep consistent with class="js-foo foo" lets keep em seprated and not put that in here

mortendk’s picture

Issue summary: View changes

updates summary

mortendk’s picture

Issue tags: -Needs issue summary update
FileSize
17.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,705 pass(es), 1 fail(s), and 0 exception(s). View

Removed the js-indentation its done over in #2449445: Add "indentation" class back to indentation theme hook, use it for styling
should be ready to go

mortendk’s picture

Issue tags: -Needs change record
mortendk’s picture

Status: Needs work » Needs review
davidhernandez’s picture

Issue summary: View changes
davidhernandez’s picture

Issue tags: -Needs beta evaluation

Status: Needs review » Needs work

The last submitted patch, 56: form-required-prefixed-js-2431671-56.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
17.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,766 pass(es), 1 fail(s), and 0 exception(s). View

sigh removed a test for the indentation ...

Status: Needs review » Needs work

The last submitted patch, 62: form-required-prefixed-js-2431671-62.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
16.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,782 pass(es). View

now no more indentation mess :/

mortendk’s picture

Title: rewrite form javascript to follow css standards » add in js- prefixed classes to forms for seperation of js & css functionality
Issue summary: View changes
Cottser’s picture

Title: add in js- prefixed classes to forms for seperation of js & css functionality » Add in js- prefixed classes to forms for separation of JS & CSS functionality
Issue summary: View changes
Status: Needs review » Needs work
  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"?

mortendk’s picture

Issue summary: View changes
mortendk’s picture

@cottset man i hate when your right ;)

Cottser’s picture

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 :)

mortendk’s picture

Status: Needs work » Needs review
FileSize
53.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch form-prefixed-js-2431671-70.patch. Unable to apply patch. See the log in the details link for more information. View

oooh man this is growing ;)

Status: Needs review » Needs work

The last submitted patch, 70: form-prefixed-js-2431671-70.patch, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
56.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,111 pass(es), 2 fail(s), and 0 exception(s). View

rerolled

Status: Needs review » Needs work

The last submitted patch, 72: form-prefixed-js-2431671-72.patch, failed testing.

mortendk’s picture

FileSize
56.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,128 pass(es). View

fixing test

mortendk’s picture

Status: Needs work » Needs review
rteijeiro’s picture

FileSize
56.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,157 pass(es). View

Re-rolled!

Cottser’s picture

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.

mortendk’s picture

@cottser im not sure it would make it easier for us to split it up - Its allready a pita as it is to get all around any forms, everytime we add on class in one place, it have to be added 200 other places, im afraid this is the easier way to do it & get it all done in one go, splitting it up i can see a huge amount of fun where we end up rerolling forever ;)
get this one in and the do followup as we find classes that needs a kiss with js- ?

Cottser’s picture

Assigned: Unassigned » Cottser

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.

mortendk’s picture

coool then lets split it up - cause yeah its huuuuuge (and a pita)
wheres the new issues then ;)

Cottser’s picture

Will update here once done :)

nod_’s picture

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.

Cottser’s picture

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.

Cottser’s picture

FileSize
56.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,428 pass(es), 2 fail(s), and 0 exception(s). View

First a reroll.

Status: Needs review » Needs work

The last submitted patch, 84: 2431671-84.patch, failed testing.

Cottser’s picture

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).

Cottser’s picture

Title: Add in js- prefixed classes to forms for separation of JS & CSS functionality » [meta] Add in js- prefixed classes for separation of JS & CSS functionality
Component: forms system » javascript
Assigned: Cottser » Unassigned
Issue summary: View changes
Status: Needs work » Active
FileSize
1.89 KB

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.

alexpott’s picture

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.

Cottser’s picture

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?

davidhernandez’s picture

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.

davidhernandez’s picture

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.

mortendk’s picture

@david We talked about this a couple of times, my fear is that it will take forever to do that, the patch is allready huge.
We wanna go for the win here that gives us the seperation thats so important for the banana & headaches all over the frontend world, creating data-attributes we can then do later, when a solution is found for that

@alex yes themers can do dumb things & we can't stop em. but if provided with the right tools we can prevent much of the dumb things ;)

ksenzee’s picture

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.

alexpott’s picture

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

lauriii’s picture

Do we have consensus on the approach?

davidhernandez’s picture

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">

nod_’s picture

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.

davidhernandez’s picture

Status: Active » Closed (fixed)

All done!

nod_’s picture

Status: Closed (fixed) » Active

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

davidhernandez’s picture

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.

LewisNyman’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Version: 8.2.x-dev » 8.3.x-dev

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.