Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Image compression #688

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[WIP] Image compression #688

wants to merge 5 commits into from

Conversation

dusanradovanovic
Copy link
Contributor

Description:

I've converted all home-page images to jpeg format and compressed them without changing resolution. I was able to reduce their file size 8x~10x without losing visual fidelity. Here is the comparison of the Lighthouse report:

image

Linting:

  • No linting errors

Tests:

  • E2E tests (npm test run with e2e)
  • Manual tests

Documentation:

  • Requires documentation updates

import productImage5 from '../../../images/site-images/b2c-product-5.png';
import productImage6 from '../../../images/site-images/b2c-product-6.png';
import productImage7 from '../../../images/site-images/b2c-product-7.png';
import bannerImage1 from '../../../images/site-images/optimized/b2c-banner-1.jpg';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to rename the file extension. ImageContainer should be fetching the allowed image types from the config, and displaying the fall-back filetypes if the preceding doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do decide to go with jpgs though our fallback imgs should be changed to jpgs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this was more of a POC to demo different image compression ratios.

@@ -2,7 +2,7 @@
"cortexApi": {
"path": "/cortex",
"scope": "vestri",
"pathForProxy": "http://localhost:9080",
"pathForProxy": "https://reference.epdemos.com",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover config

@shaunmaharaj
Copy link
Contributor

shaunmaharaj commented May 5, 2020

@dusanradovanovic moving to lossy JPEG is not going to be ideal for us as the quality of these images will suffer 😞 This is why we've been keeping png thus far, and compressing any new images with https://tinypng.com/.

Comparing a few of the converted images with their compressed png counterparts, there's a noticeable degradation of quality during compression (example: b2c-product-3.jpg, b2c-product-6.png).

Some options:

  • We could convert certain images to JPEG that we don't need to be scaled at higher resolutions (smaller marketing spots, etc).
  • Continue work with showing selective images at various breakpoints (srcset) and have different sized images for each breakpoint.

FYI @BonnieEP

@shaunmaharaj shaunmaharaj mentioned this pull request May 5, 2020
4 tasks
@aChanEP
Copy link
Contributor

aChanEP commented May 5, 2020

@dusanradovanovic moving to lossy JPEG is not going to be ideal for us as the quality of these images will suffer 😞 This is why we've been keeping png thus far, and compressing any new images with https://tinypng.com/.

Comparing a few of the converted images with their compressed png counterparts, there's a noticeable degradation of quality during compression (example: b2c-product-3.jpg, b2c-product-6.png).

Some options:

  • We could convert certain images to JPEG that we don't need to be scaled at higher resolutions (smaller marketing spots, etc).
  • Continue work with showing selective images at various breakpoints (srcset) and have different sized images for each breakpoint.

FYI @BonnieEP

@dusanradovanovic moving to lossy JPEG is not going to be ideal for us as the quality of these images will suffer 😞 This is why we've been keeping png thus far, and compressing any new images with https://tinypng.com/.

Comparing a few of the converted images with their compressed png counterparts, there's a noticeable degradation of quality during compression (example: b2c-product-3.jpg, b2c-product-6.png).

Some options:

  • We could convert certain images to JPEG that we don't need to be scaled at higher resolutions (smaller marketing spots, etc).
  • Continue work with showing selective images at various breakpoints (srcset) and have different sized images for each breakpoint.

FYI @BonnieEP

I am not seeing a huge difference between the png's beside the jpg's. From what ive been reading Jpgs are meant for photographic images where as pngs are more meant for graphical images where you need solid crisp borders. I agree with Shaun though that if the degredation is not acceptable we should change some of our less important and smaller images to be jpg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants