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

Reference: https://www.drupal.org/core/beta-changes
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.

Files: 
CommentFileSizeAuthor
#25 Screen Shot 2015-05-10 at 16.38.50.png203.77 KBemma.maria
#25 Screen Shot 2015-05-10 at 16.37.48.png151.49 KBemma.maria
#24 interdiff.txt862 bytesCottser
#24 2449445-24.patch3.86 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,488 pass(es). View
#23 2449445-23.patch3.32 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,318 pass(es). View
#23 interdiff.txt697 bytesCottser
#14 d8.indentation-template-5.patch2.64 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,258 pass(es). View
#11 d8.indentation-template-4.patch1.11 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,123 pass(es), 2 fail(s), and 0 exception(s). View
#5 d8.indentation-template-3.patch641 bytesmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,123 pass(es). View
d8.indentation-template.patch629 bytesalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,127 pass(es). View

Comments

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks good and is a straight bug.

Cottser’s picture

mortendk’s picture

Status: Reviewed & tested by the community » Needs work

misses the indentation class we use for css

mortendk’s picture

Status: Needs work » Reviewed & tested by the community

im an idiot - offcourse its not needed in core as people can add what they want dooh ... carry on ;)

mortendk’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
641 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,123 pass(es). View

discussed 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 indentation

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, the remove of the class can happen later.

Cottser’s picture

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

mortendk’s picture

@cottser afaik the theme function allready do that - we just forgot this template on the original patch

Cottser’s picture

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

mortendk’s picture

Status: Reviewed & tested by the community » Needs work

@cottser so should we roll that into this issue as well
sounds like it ?

mortendk’s picture

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

now js-indentation indentation both in the theme function and the example

Status: Needs review » Needs work

The last submitted patch, 11: d8.indentation-template-4.patch, failed testing.

Cottser’s picture

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

mortendk’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,258 pass(es). View

ok fixed test & changed the css as well

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, .js- in .css was just feeling ... wrong.

Nice!

Cottser’s picture

Title: Prefix indentation class with js in twig template » Add "indentation" class back to indentation theme hook, use it for styling
Issue summary: View changes
Issue tags: -Quickfix

Updating title, issue summary, adding beta evaluation. +1 to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 4fd9ea4 on 8.0.x
    Issue #2449445 by mortendk, alexpott, Cottser: Add "indentation" class...
nod_’s picture

Issue tags: +JavaScript

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.

TR’s picture

Priority: Normal » Major
Status: Fixed » Active
Related issues: +#2426595: Rename indentation class to js-indentation

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

  • webchick committed 854cfcf on
    Revert "Issue #2449445 by mortendk, alexpott, Cottser: Add "indentation...
webchick’s picture

Status: Active » Needs work

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

Cottser’s picture

Priority: Major » Normal
Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
697 bytes
3.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,318 pass(es). View

Yup, this missed a spot. Attached should be better.

Cottser’s picture

FileSize
3.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,488 pass(es). View
862 bytes

And to make the test more accurate/up to date…

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
151.49 KB
203.77 KB

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 14ae16f and pushed to 8.0.x. Thanks!

  • alexpott committed 14ae16f on 8.0.x
    Issue #2449445 by Cottser, emma.maria: Add "indentation" class back to...
mortendk’s picture

tsk tsk tsk i dont get attribute love for this cause i created it ;)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.