-
Notifications
You must be signed in to change notification settings - Fork 982
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
Reduce image sizes #20076
Reduce image sizes #20076
Conversation
Jenkins BuildsClick to see older builds (30)
|
351751f
to
d02f6bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice
@Francesca-G I did some work to compress these images, it would help in reducing the size of the application, i think this needs a design review to just be sure you are okay with the quality of the compression. have a look whenever you can |
85% of end-end tests have passed
Failed tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (44)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jo-mut, could you share which commands you used to compress PNG files?
WebP should be smaller for the same quality, ~25% less, but I checked that for rasterized images it may not be the best format, which explains the results you reported. I could create smaller WebP files than the PNG ones, but they introduced too many artifacts to my liking, visibly worse than the PNGs. The AVIF format on the other hand generates significantly smaller files than WebP for the same quality, but they still generate visible artifacts to achieve the same file size as the PNGs you generated.
Ideally we should have a quick script to compress a list of files or a directory. This script would have been useful now and in future iterations. If the script is too much to ask, could you at least add the commands in the PR description you used to generate these files?
Great work! 🚀
Edit: To be fair to other image formats and the quality of the final output, ideally the conversion should start from the original files and not go through original -> PNG -> WebP.
I used an online tool, tinypng.com. I could try do some more work see if i
can get any better results with webp or avifs.
I think a script for this kind of thing would do better for any future use
as the designs come
…On Fri, 17 May 2024, 16:48 Icaro Motta, ***@***.***> wrote:
***@***.**** approved this pull request.
@jo-mut <https://github.com/jo-mut>, could you share which commands you
used to compress PNG files?
WebP should be smaller for the same quality, ~25% less, but I checked that
for rasterized images it may not be the best format, which explains the
results you reported. I could create smaller WebP files than the PNG ones,
but they introduced too many artifacts to my liking, visibly worse than the
PNGs. The AVIF format on the other hand generates significantly smaller
files than WebP for the same quality, but they still generate visible
artifacts to achieve the same file size as the PNGs you generated.
Ideally we should have a quick script to compress a list of files or a
directory. This script would have been useful now and in future iterations.
If the script is too much to ask, could you at least add the commands in
the PR description you used to generate these files?
Great work! 🚀
—
Reply to this email directly, view it on GitHub
<#20076 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AG27564M5YXHWELLL5OFBTDZCYKENAVCNFSM6AAAAABH3DW5MGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANRTGY2DEOBWGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Images looks sharp to me (iPhone 13)
as mentioned here on Discord we should only use a static illustration (no transition) on the Generating Keys screens, please make sure to address this in a separate issue 🙏
It would be very helpful @jo-mut to have a basic script because we will surely have to do this again. It will save us time if you could do that. And it would help others reproduce and play with parameters.
I already tried using imagemagick's I just tried const sharp = require("sharp");
sharp("generating-keys-1@2x.png")
.png({
effort: 10,
quality: 50, // We can play with this number
compressionLevel: 9
palette: true // This is critical to lower file sizes
})
.toFile("generating-keys-1@2x.png - compressed")
.then((info) => {
console.log("Image compression completed:", info);
})
.catch((err) => {
console.error("Error during image compression:", err);
}); |
Given the good results obtained by Well, I'm just giving options @jo-mut. And this topic about image compression is something I did a ton in life, so that's why I tried to dig deeper in this PR. |
@ilmotta I agree with you, no need to merge now. I can play with this sharp script. Let me see how much we can achieve if i convert all the images in Ui2 |
@mariia-skrypnyk not yet, i will might take some more time to work on this. Am currently on time off. We can revisit on wednesday |
Thanks @jo-mut ! Just tag me and I'll be notified to test your PR. |
8f03ac8
to
605c168
Compare
Thanks for trying the different approach @jo-mut. At least now we know we have alternatives that would give competitive asset sizes compared to tinypng and which are open-source. |
605c168
to
3540de4
Compare
@ilmotta I think this is even better, the difference between this approach and tinypng if there is even any is hard to tell, the quality of images is pretty much the same and with the script it achieves even smaller sizes This pr can be merged I will add the script to our codebase in a separate pr. Thank you for the suggestion |
@mariia-skrypnyk Hi maria. When you have some time you can take another look at this pr. |
Please, rebase your PR. |
3540de4
to
98ad4ce
Compare
Hey @jo-mut ! Thanks for your PR. A lot of work was done! Going to move PR to @Francesca-G review as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding follow up required label to address this issue 👍
@jo-mut You can merge your PR after creating followup @Francesca-G mentioned above 🙌. |
98ad4ce
to
818a1e6
Compare
818a1e6
to
069bbe2
Compare
|
069bbe2
to
8574a63
Compare
fixes #20075
Summary
In this pr, we try to reduce the size of the images used especially in the onboarding process to help in reducing the size of the application.
This is just a sample of images. We could achieve much more space by simply reducing the size of all images. In this pr, all images with over 100kb in Ui2. Combine they had a sum total of 26mb and its been reduced to 7.2mb.
status: wip