(First, verify that the preprocess changes have been made. #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.)

  1. Copy the Twig templates from the core module's templates directory to Classy's templates directory. Include all templates, even ones without classes.
  2. Remove all classes from the core module's template. Remove all classes added with addClass and ones that are hard-coded in the template.
  • If there are classes that are required for basic functionality, discuss whether they should be kept.
  • If there is CSS from the module, or anywhere else, referring to the class, discuss removing it or moving it to Bartik&Seven. Do not move the CSS to Classy.

Twig Templates to Copy

core/modules/image/templates/image-anchor.html.twig
core/modules/image/templates/image-crop-summary.html.twig
core/modules/image/templates/image-formatter.html.twig
core/modules/image/templates/image-resize-summary.html.twig
core/modules/image/templates/image-rotate-summary.html.twig
core/modules/image/templates/image-scale-summary.html.twig
core/modules/image/templates/image-style-preview.html.twig
core/modules/image/templates/image-style.html.twig
core/modules/image/templates/image-widget.html.twig

Files: 
CommentFileSizeAuthor
#24 Screenshot 2015-01-20 17.22.52.jpg444.35 KBLewisNyman
#24 Screenshot 2015-01-20 17.20.42.jpg406.56 KBLewisNyman
#16 copy_image_templates_to-2349687-16.patch10.13 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,077 pass(es). View
#15 copy_image_templates_to-2349687-15.patch10.13 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,424 pass(es). View

Comments

mortendk’s picture

FileSize
9.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,916 pass(es). View

moved the classes out of core + all template files copied over
Discussion of the classes - should be done as a follow up, so bikesheedding wont stop classy ;)

mortendk’s picture

Status: Active » Needs review
Issue tags: +cssbanana

go bot go

davidhernandez’s picture

Status: Needs review » Needs work

The templates should go into the root of the templates folder, not a subfolder.

lauriii’s picture

+++ b/core/modules/image/templates/image-style-preview.html.twig
@@ -30,28 +30,28 @@
-      <div class="preview-image original-image" style="width: {{ preview.original.width }}px; height: {{ preview.original.height }}px;">
...
-      <div class="height" style="height: {{ preview.original.height }}px"><span>{{ original.height }}px</span></div>
-      <div class="width" style="width: {{ preview.original.width }}px"><span>{{ original.width }}px</span></div>
...
-      <div class="height" style="height: {{ preview.derivative.height }}px"><span>{{ derivative.height }}px</span></div>
-      <div class="width" style="width: {{ preview.derivative.width }}px"><span>{{ derivative.width }}px</span></div>

Are we really gonna remove styles from divs also? I thought classy is only about classes

derheap’s picture

The style attributes have to stay: they are necessary for the preview.

mortendk’s picture

Issue tags: +drupalhagen

yup they are nessesary and thats why they are in classy - but not in the core.

runand’s picture

Assigned: Unassigned » runand
runand’s picture

Assigned: runand » Unassigned
Status: Needs work » Needs review
FileSize
13.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,643 pass(es). View
davidhernandez’s picture

Please double-check if any removed classes are being used in javascript. It is best to test the affected template using Stark to make sure nothing is broken.

mdrummond’s picture

I think derheap has a good point. These are the templates to generate previews of how image styles will look, right? That still has to work in themes that are using Core instead of Classy without every theme manually re-implementing that.

mdrummond’s picture

Although now that I'm thinking about it, an image style preview would be showing up in Seven, which is based on Classy, so this is maybe fine.

lauriii’s picture

I dont think we should break functionality with classy

davidhernandez’s picture

The style attributes should stay in core as well as Classy. It is part of the functionality of the the template.

mortendk’s picture

ok well then lets have a follow up issue on this & get it moved over without removing the inline styles.

lauriii’s picture

FileSize
10.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,424 pass(es). View

Unremoved style attributes

mortendk’s picture

FileSize
10.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,077 pass(es). View

to prevent disucssion about if inline styles are preferable in a template i have removed that from the patch, we can do that as a followup instead .

lauriii’s picture

Could you provide interdiff?

davidhernandez’s picture

If those are copied files without changes, please check your git settings. https://www.drupal.org/documentation/git/configure . The section on "Optimize diffs for renamed and copied files".

LewisNyman’s picture

Do we want to remove these empty divs and spans in the module template files?

mortendk’s picture

We should do that as followup's lets get the templates into classy first, then we can clean up core even more if theres a need for that

davidhernandez’s picture

Why wouldn't the empty divs be removed? I'm pretty sure we've done that in other issues?

davidhernandez’s picture

and also try to avoid follow ups as much as we can.

LewisNyman’s picture

I took some screenshots before and after. It looks like the image style preview functionality is broken in core or is it just me?

mortendk’s picture

@david - didnt we end up in never ending bikesheds when we tried to do tomuch ?
keeping those things to followups, makes it easier for us to move stuff into classy & not beeing borked down.
Im pretty sure we did that on other issues before, but if its not gonna be suddenly blockers, then yes for all sake lets clean as much as possible :)

davidhernandez’s picture

I thought we did, but now I don't know. I'll look through the previous ones to see what we decided.

davidhernandez’s picture

I think you're right. The other ones left empty divs, so we must have left that for "dream markup". Ignore me, carry on.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Ok thanks for the replies. I can confirm that the image style styling is broken in HEAD so we need to open a new issue], not fix it here.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  • If there are classes that are required for basic functionality, discuss whether they should be kept.
  • If there is CSS from the module, or anywhere else, referring to the class, discuss removing it or moving it to Bartik&Seven. Do not move the CSS to Classy.

This bit from the IS does not seem to have been done.

Manuel Garcia’s picture

I have greped core for the classes that we are removing from image module templates, which are:

image-style-preview
preview
clearfix
preview-image-wrapper
preview-image
original-image
modified-image
height
width
image-preview
image-widget-data

There are no JS files in core that make use of any of these css classes that I could find.

As far as CSS goes, the ones I found are all in the module's CSS files: image.admin.css and image.theme.css. Please note that I ignored clearfix class because this is just a helper class.

On image.admin.css:
These are used (moved to classy in the patch)

  • .image-style-preview
  • .preview-image-wrapper
  • .preview-image
  • .width (!!)
  • .height (!!)

On image.theme.css:
These are used:

  • .image-preview
  • .image-widget-data

These are NOT used anywhere (should be safe to remove)

  • .preview
  • .original-image
  • .modified-image
mortendk’s picture

Status: Needs work » Reviewed & tested by the community

ill set this back to rtbc - we are not cleaning out css nor are we yet moving css over.
Anyways templates are ready to be moved, css have been discussed & we have an overview :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Templates changes are not blocked during beta. Committed 16d3e1c and pushed to 8.0.x. Thanks!

  • alexpott committed 16d3e1c on 8.0.x
    Issue #2349687 by mortendk, LewisNyman, lauriii, runand: Copy image...

Status: Fixed » Closed (fixed)

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