Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pere Orga’s picture

Status: Needs work » Needs review
FileSize
14.44 KB

Patch attached.

For the PNGs, I've used the Zopfli library.
For the GIF, I've used Gifsicle and Imagemagick.

See the same commit on github: https://github.com/pereorga/commerce/commit/dd6968e1a573c75cbd62e6fdc3eb...

Pere Orga’s picture

Status: Needs review » Needs work

Sorry it's wrong

Pere Orga’s picture

Status: Needs work » Needs review
FileSize
15.59 KB

For some reason, modules/cart/theme/buttons.png was compressed lossy.

Fixed patch attached. https://github.com/pereorga/commerce/commit/0e0b389b3233dc07ff63bafd989c...

Pere Orga’s picture

Issue summary: View changes

Fix grammar

joelpittet’s picture

Issue summary: View changes

@Pere Orga Is there anything special needed to apply this patch?

Patch throws back:
patch: **** Only garbage was found in the patch input.

Pere Orga’s picture

It should work with git apply --binary 2068351-Optimize-images-using-lossless-compression-3.patch

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Performance
FileSize
32.43 KB

Thanks @Pere Orga:)

Front end performance improvement!

But unfortunately they are no longer transparent (see the pending payment one for example)
If they are no longer transparent they could break themes, so setting back to needs work. (needs lesslessness)

The other ones look good I think, but worth another look.

Pere Orga’s picture

joelpittet’s picture

@Pere Orga is that missing from patch in #3 maybe?

Forgot to attach my proof:P

I think all the other ones are fine. It's obvious by the diff stats too that that got quite a substantial reduction than the others.

Pere Orga’s picture

FileSize
2.99 KB
232 bytes

I think it's your image viewer, but it's a concern if the alpha channel is encoded with less compatibility or something. Attaching both files.

joelpittet’s picture

@Pere Orga, not my image viewer (Photoshop CS6), you can clearly see that it's different from the other files by the amount of reduction in file size compared to the others. AKA somethings up even without looking at the image, I just opened that image to see why, and removed transparency is why.

Opened the second png in #9 same deal, same missing transparency.

joelpittet’s picture

FYI, my imageOptim software package does that to it too FWIW.

joelpittet’s picture

Issue summary: View changes
Priority: Minor » Normal
Status: Needs work » Needs review
FileSize
45.13 KB
16.99 KB

re-saved it as a PNG-24 from PS and the reduction though not as much 378bytes vs 232bytes in yours. As soon as it's run through the compression it's likely too small of an image or the transparency is too subtle that it disregards it.

Slight improvement on some of the others as well:

joelpittet’s picture

@Pere Orga could you check my buttons.png, I think it may have shifted colors.

Pere Orga’s picture

@Pere Orga, not my image viewer (Photoshop CS6), you can clearly see that it's different from the other files by the amount of reduction in file size compared to the others. AKA somethings up even without looking at the image, I just opened that image to see why, and removed transparency is why.

Files look exactly the same to me on Chrome. The size difference is not necessarily meaningful here, as the source files may be more or less optimized (btw good to know that ImageOptim alone is doing a better job.)

@Pere Orga could you check my buttons.png, I think it may have shifted colors.

They look the same to me.

But patch contains extra changes:

M	modules/customer/includes/commerce_customer_profile.controller.inc
M	modules/customer/tests/commerce_customer_ui.test
joelpittet’s picture

Status: Needs review » Needs work

oh sorry, I had extra crap from another issue in my patching repo:S Thanks for noticing.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +frontend performance, +commerce-sprint
FileSize
13.99 KB

Removed extra files.

nvahalik’s picture

While I like this general idea, two general observations:

1. If you're going for efficiency, then why not just use "✓" "✕" and "◷" with some CSS? This way you eliminate the requests for each of the images and you'll save quite a bit more space. Plus, it scales better than these existing images which leads me to...
2. These images look bad on Hi-DPI screens, they are currently 12x12 each. If you are intent on using images, then I'd suggest at least updating them to be higher resolution and/or making them a sprite. I've attached some 32x32 icons I build using Iconic's open icon set. This image is 1108 bytes (so it's ~220 bytes bigger) but the icons nearly 3x the original size. You could chop them up or go even bigger if you wanted. I used the circle-check, circle-x, and clock icons.

joelpittet’s picture

Issue tags: +Quick fix

The text glyphs don't look that good and can change between browsers/systems. We'd be better of with SVG. This is to not affect existing sites, just improve what we have.

This is really a quick fix. Maybe we can open a new image for some SVGS. Iconic looks like they have a good license.

Hubbs’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
57.84 KB
201.52 KB

Just reviewed this patch and I can confirm that the image files are both smaller in file size and transparent (images below).

optimized images

transparent images

I agree with nvahalik that some SVG or icon-fonts would be better for this overall. Another issue though.

joelpittet’s picture

thanks @Hubbs and @nvahalik. Here is a follow-up to replace them. #2625938: Replace image icons with SVG for HiDPI devices

Hopefully we can just commit this as-is and push forward on the other issue till it's good.

rszrama’s picture

Status: Reviewed & tested by the community » Fixed

I've committed this, but I really don't see the need for optimizing this. We're talking about shaving 2k off an image we copied from core Drupal for no perceivable benefit. I suppose the follow-up will be more meaningful, but I'm just committing this to get it out of RTBC. : P

  • rszrama committed 20c8e8a on 7.x-1.x authored by Pere Orga
    Issue #2068351 by joelpittet, Pere Orga: Optimize images using lossless...
joelpittet’s picture

Yes! pity commit:D

rszrama’s picture

hehehe, I realized I was spending more time trying to figure out the benefit than it really deserved. Getting images that work in retina displays will be nice, though!

Pere Orga’s picture

Drupal Commerce have only a few images and there were already optimized. I think it's a good practice to keep images optimized, but I agree here there was really no need (I hope it didn't hurt, though)

On Omega theme, for example, optimizing images saved 592KB: https://www.drupal.org/node/2064651

rszrama’s picture

Wow, that's crazy! Definitely worth doing. : )

Status: Fixed » Closed (fixed)

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