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

Lossless compression of all images #11417

Merged
merged 4 commits into from Nov 19, 2018

Conversation

Projects
None yet
7 participants
@MathiasReker
Contributor

MathiasReker commented Nov 16, 2018

Questions Answers
Branch? develop
Description? Lossless conpression of all images.
Type? improvement
Category? CO
BC breaks? Does it break backward compatibility? no
Deprecations? Does it deprecate an existing feature? no
Fixed ticket? /
How to test? /

This change is Reviewable

@MathiasReker

This comment has been minimized.

Contributor

MathiasReker commented Nov 16, 2018

Saved about 2 MB

@MathiasReker

This comment has been minimized.

Contributor

MathiasReker commented Nov 16, 2018

I tried to fix the conflicts, but I have some troubles with it. Are those images generated? I can't see the conflict.

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 16, 2018

I like the idea :)
What kind of command do you use to compress file?
@TristanLDD Can you have a check on these files?

@MathiasReker

This comment has been minimized.

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Nov 16, 2018

Good to know :D Waiting for UX designers to have a check if everything is ok. But I totally approve the change!
For public/* images, I don't know if they are generated or not :(

@MathiasReker

This comment has been minimized.

Contributor

MathiasReker commented Nov 16, 2018

It took me 5 hours and 15 min. to scan all files and compress the images. Fileoptimizer uses several image optimization scripts on each file. You can see the list of used scripts in the link just I sent.

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Nov 16, 2018

You should probably avoid committing the files in admin-dev/themes/new-theme/public/ as they will be replaced with the next assets generation . :)

@MathiasReker

This comment has been minimized.

Contributor

MathiasReker commented Nov 16, 2018

Can they stay for now and just get overrided?

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Nov 16, 2018

As you wish, but you need anyway to solve the conflicts. :)

@TristanLDD

This comment has been minimized.

TristanLDD commented Nov 16, 2018

Everything is ok for me !

@TristanLDD TristanLDD added UX ✔️ and removed waiting for UX labels Nov 16, 2018

@MathiasReker

This comment has been minimized.

Contributor

MathiasReker commented Nov 16, 2018

I need help to resolve the conflicts. From my commits, you can see I tried to revert the images caused the conflict, but I had some troubles with it. I can't click the "Resolve conflicts" button. because: "These conflicts are too complex to resolve in the web editor"

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Nov 19, 2018

Hello @MathiasReker,

let's do it the easy way, revert changes done in admin-dev/themes/new-theme/public/ folder, imho the optimizations should be done on this folder using webpack.

Regards,

@Quetzacoalt91 Quetzacoalt91 dismissed stale reviews from PierreRambaud and themself via da55c1c Nov 19, 2018

@Quetzacoalt91 Quetzacoalt91 force-pushed the MathiasReker:optimize-images branch from 34d9188 to da55c1c Nov 19, 2018

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Nov 19, 2018

I updated the pull-request to remove the conflicted files and trigger a Travis build. :)

@MathiasReker

This comment has been minimized.

Contributor

MathiasReker commented Nov 19, 2018

Thanks @Quetzacoalt91 =)

@marionf marionf self-assigned this Nov 19, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 19, 2018

@marionf marionf removed their assignment Nov 19, 2018

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.6.0 milestone Nov 19, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit 2da8291 into PrestaShop:develop Nov 19, 2018

1 of 2 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Nov 19, 2018

Thank you @MathiasReker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment