Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
#2426595: Rename indentation class to js-indentation prefixed the "indentation" class with "js-". It feels weird to then style this.
Also, the same issue forgot to update the Twig template that is not used by default, indentation.html.twig.
Proposed resolution
Add back "indentation" and use it for styling. Sync up the theme function output with the Twig template.
Remaining tasks
None at the moment.
Beta phase evaluation
| Issue category | Bug because the Twig template is out of sync |
|---|---|
| Issue priority | Normal, the Twig template is not used out of the box. |
| Unfrozen changes | Unfrozen because it only changes a theme function, a Twig template, CSS, and automated tests. |
| Prioritized changes | This is not a prioritized change for the beta phase. |
| Disruption | Not disruptive for core or contrib. |
User interface changes
Fixed styling of indentation if you are using the indentation.html.twig override and update your template override.
API changes
None.
Original report by @alexpott
#2426595: Rename indentation class to js-indentation forgot to update the example twig template.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | Screen Shot 2015-05-10 at 16.38.50.png | 203.77 KB | emma.maria |
| #25 | Screen Shot 2015-05-10 at 16.37.48.png | 151.49 KB | emma.maria |
| #24 | interdiff.txt | 862 bytes | star-szr |
| #24 | 2449445-24.patch | 3.86 KB | star-szr |
| #23 | 2449445-23.patch | 3.32 KB | star-szr |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedRTBC, looks good and is a straight bug.
Comment #2
star-szr+1, thanks @alexpott!
Comment #3
mortendk CreditAttribution: mortendk commentedmisses the indentation class we use for css
Comment #4
mortendk CreditAttribution: mortendk commentedim an idiot - offcourse its not needed in core as people can add what they want dooh ... carry on ;)
Comment #5
mortendk CreditAttribution: mortendk commenteddiscussed with cottser at #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality and it make sense to keep both classes so were at par with what comes out of core by default
js-indentation should be added & not replace
class="js-indentation indentationComment #6
Fabianx CreditAttribution: Fabianx commentedBack to RTBC, the remove of the class can happen later.
Comment #7
star-szrI'm not as sure about that as a standalone change, I think the original patch is better because it at least lines up with the current theme function's output. The template from #5 would be more appropriate for #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality IMO, including changing the theme function output to match that.
So I'd rather see patch #0 committed here :)
Comment #8
mortendk CreditAttribution: mortendk commented@cottser afaik the theme function allready do that - we just forgot this template on the original patch
Comment #9
star-szrWhat I'm saying is theme_indentation() in HEAD outputs only "js-indentation", so for this patch to make the template output "js-indentation indentation" is not consistent. Best to leave that change to the related issue.
Comment #10
mortendk CreditAttribution: mortendk commented@cottser so should we roll that into this issue as well
sounds like it ?
Comment #11
mortendk CreditAttribution: mortendk commentednow
js-indentation indentationboth in the theme function and the exampleComment #13
star-szrYeah, sure… at that point it might make sense to change the CSS too though (that's currently targeting .js-indentation) :/
And tests need to be updated.
So it becomes less of a quickfix thing.
Comment #14
mortendk CreditAttribution: mortendk commentedok fixed test & changed the css as well
Comment #15
Fabianx CreditAttribution: Fabianx commentedRTBC, .js- in .css was just feeling ... wrong.
Nice!
Comment #16
star-szrUpdating title, issue summary, adding beta evaluation. +1 to RTBC.
Comment #17
webchickCommitted and pushed to 8.0.x. Thanks!
Comment #19
nod_Massive JS change, please remember to tag it with JavaScript.
Sorry this issue is not a JS change, got mixed up. Leaving the tag though, it's relevant.
Comment #20
TR CreditAttribution: TR commentedThis commit broke tabledrag functionality.
Reverting this commit fixes the way tabledrag works.
For example, when editing a taxonomy vocabulary, try dragging a term to the right to change the level the term has in the hierarchy. A
<div class="js-indentation">gets added which is supposed to indent the term, instead it adds space above the term in the row but doesn't change its indentation. After saving the table, the term *does* show the proper indentation, but at this point the markup contains<div class="js-indentation indentation">. TaxonomyTermIndentationTest only checks the end result of the drag after save, so doesn't/can't catch this.Renaming .js-indentation to .indentation in system.module.css, as was done by this issue, removes the CSS needed by tabledrag to function.
Comment #22
webchickWell! Let's roll that back then. Thanks a lot for tracking this down. I noticed this was broken the other day but didn't have time to look into it more.
Comment #23
star-szrYup, this missed a spot. Attached should be better.
Comment #24
star-szrAnd to make the test more accurate/up to date…
Comment #25
emma.mariaI have tested the menu drag functionality with the patch in #24 applied.
The table drag functionality works as it should.
See screenshots...
I declare this issue RTBC.
Comment #26
alexpottCommitted 14ae16f and pushed to 8.0.x. Thanks!
Comment #28
mortendk CreditAttribution: mortendk commentedtsk tsk tsk i dont get attribute love for this cause i created it ;)