Problem/Motivation

Remove non essential classes from core - the classes are in classy, cores templates should be as clean as possible.
js required classes should be prefixed to js- according to code standards
simpletest should be done with classy theme, not stark

See parent issue(s) and related issue(s) for details,

Proposed resolution

Twig Templates to cleanup

  • core/modules/system/templates/image.html.twig
  • core/modules/system/templates/indentation.html.twig
  • core/modules/system/templates/input.html.twig
  • core/modules/system/templates/install-page.html.twig install-page was not copied to Classy. Do not modify it.
  • core/modules/system/templates/item-list.html.twig

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#21 image-before.png308.19 KBrteijeiro
#21 image-after.png308.19 KBrteijeiro
#20 cleanup-templates-i-3.diff1.72 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,025 pass(es). View
#19 cleanup-templates-i-2.diff873 bytesmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,798 pass(es). View
#7 cleanup-templates-i.diff1.44 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,755 pass(es). View
#2 issue-2407733-2.patch7.22 KBSivaji
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,895 pass(es). View

Comments

Sivaji’s picture

Issue summary: View changes
Sivaji’s picture

Status: Active » Needs review
FileSize
7.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,895 pass(es). View
davidhernandez’s picture

Status: Needs review » Postponed

Postponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the Classy theme for updates. Thanks.

davidhernandez’s picture

Title: Copy system templates i*.html.twig to Classy » Remove classes from system templates i*.html.twig
Issue summary: View changes
Status: Postponed » Needs work

Un-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.

davidhernandez’s picture

The "set classes" is not indented correctly in the template. I wonder if we can fix that in this issue. It is a minor thing.

mortendk’s picture

Issue summary: View changes
mortendk’s picture

Issue summary: View changes
FileSize
1.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,755 pass(es). View
mortendk’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: cleanup-templates-i.diff, failed testing.

Status: Needs work » Needs review

mortendk queued 7: cleanup-templates-i.diff for re-testing.

mortendk’s picture

Issue tags: +drupalcafe-feb2015
kasperg’s picture

Assigned: Unassigned » kasperg

I'll take a stab at reviewing this.

kasperg’s picture

Assigned: kasperg » Unassigned
Status: Needs review » Reviewed & tested by the community

I have reviewed and tested the patch and it works as expected.

Classes are removed from item lists and images when using image styles.

I found changes to indentation.html.twig hard to test since theme_indentation is used by default. Copying the contents of the template to classys indentation.html.twig and rendering the admin menu tree for classy shows that it works.

davidhernandez’s picture

Status: Reviewed & tested by the community » Needs work

I think the class and blank space shouldn't be removed from the indentation template. That is essentially its whole purpose. Without it you are running through a 'for' loop that just prints an empty div.

mortendk’s picture

printing a &nsp; is not very 2015 its more like 2001.
We remove any class that is not essential to the functionality.
An indent is a visual enhancement, thats not essential for the module / template to work

mortendk’s picture

Status: Needs work » Reviewed & tested by the community
mortendk’s picture

Status: Reviewed & tested by the community » Needs review

i cant set my own stuff to rtbc *doh*

davidhernandez’s picture

I think the indentation class is essential to the templates functionality. Otherwise, what is it doing? You have a 'for' loop printing a series of empty divs. And in the land of dream markup, the divs would get removed, so then it isn't doing anything. It's a very 2001 thing, but that seems to be its purpose.

mortendk’s picture

FileSize
873 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,798 pass(es). View

@david you correct, actually we should not even have the template in classy, im creating a followup issue for this, as well as a prober naming for the .indentation class as it shold be .js-indentation

mortendk’s picture

FileSize
1.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,025 pass(es). View

on the other hand lets just kill off indentation.html.twig right away

rteijeiro’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
308.19 KB
308.19 KB

Good! Well done @mortendk!!

Image BEFORE

Image AFTER

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So copying indentation.html.twig to classy was wrong since now it is being used - contrary to the comments. The correct thing to do here is to remove the it from classy. And not worry about the one in system since it is not in used - as per the comment.

davidhernandez’s picture

Isn't that what the patch in #20 does? Removes it for Classy.

mortendk’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -drupalcafe-feb2015

@alex the fil got removed from classy in #20 - setting it back to rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Templates are not frozen in beta. Committed a00b10d and pushed to 8.0.x. Thanks!

  • alexpott committed a00b10d on 8.0.x
    Issue #2407733 by mortendk, rteijeiro, sivaji@knackforge.com: Remove...

Status: Fixed » Closed (fixed)

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