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.

Sign up for GitHub

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

Jump to bottom

[NEW] Accept GIFs and SVGs for Avatars converting them to PNG and keep transparency of PNGs #11385

Merged
merged 5 commits into from
Oct 20, 2019
Merged

[NEW] Accept GIFs and SVGs for Avatars converting them to PNG and keep transparency of PNGs #11385

merged 5 commits into from
Oct 20, 2019

Conversation

tassoevan
Copy link
Contributor

@tassoevan tassoevan commented Jul 7, 2018 •
edited by engelgabriel
Loading

Closes #11230
Closes #3766

avatars

@tassoevan tassoevan added the ui/ux label Jul 7, 2018
@tassoevan tassoevan requested review from rodrigok and ggazzo July 7, 2018 05:42
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11385 July 7, 2018 05:43 Inactive
vynmera
vynmera previously approved these changes Jul 7, 2018
View reviewed changes
@rodrigok
Copy link
Member

rodrigok commented Jul 9, 2018

@tassoevan what is the difference in size (kb) of the final images? (jpeg x png)

@rafaelks is that change (from jpeg to png) ok for mobiles?

@tassoevan
Copy link
Contributor Author

@rodrigok Using rocketcat.png as reference, the cropped PNG version is twice the size of the JPEG version. Using a very noisy image, PNG avatar was around seven times greater than the JPEG one. Should I test PNG compression and apply adaptative filtering?

@rafaelks
Copy link
Contributor

rafaelks commented Jul 9, 2018

@rodrigok We're currently requesting the avatars in jpeg format via mobile... will that keep working?

"(baseURL)/avatar/(encodedUsername)?format=jpeg"

@rodrigok
Copy link
Member

rodrigok commented Jul 9, 2018

@tassoevan @rafaelks I would say we should only change the default background color to white and not allow avatars with transparency

@tassoevan
Copy link
Contributor Author

I guess this was discussed before (#3766), but I see no conclusion about it.

engelgabriel
engelgabriel previously requested changes Jul 9, 2018
View reviewed changes
Copy link
Member

@engelgabriel engelgabriel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@tassoevan can we verify the upload format and keep the same? So rather than forcing one or the other, we can allow people to decided? PNGs for photos is BAD, and JPG for logos is also really BAD.

I know that the browser can guess the image type from the file itself, but I am not sure how it would work on mobile? Maybe we can send the filetype on the HTTP headers?

What do you guys think?

@rafaelks
Copy link
Contributor

rafaelks commented Jul 9, 2018

JPEG & PNG both works in mobile with no problem at all, but we should respect the format parameter in the query always or deprecate it.

@tassoevan
Copy link
Contributor Author

@rafaelks I just changed the format which the avatars are stored. The output still converts from that format to the one requested in format parameter.

@rafaelks
Copy link
Contributor

rafaelks commented Jul 9, 2018

@tassoevan Great! So nothing should change for the mobile apps since both applications are passing the format parameter. 😊

@rodrigok
Copy link
Member

@gabrielsixel do we want to accept avatars with transparent backgrounds?

@gabrielsixel
Copy link
Contributor

@rodrigok wrong Gabriel?

@rodrigok
Copy link
Member

@gabrielsixel ops, sorry 😄

@engelgabriel do we want to accept avatars with transparent backgrounds?

@tassoevan
Copy link
Contributor Author

Better link #11202 here since there are common decisions to be made.

@ggazzo ggazzo added this to the 0.67.0 milestone Jul 17, 2018
@theorenck theorenck modified the milestones: 0.67.0, 0.68.0 Jul 19, 2018
@theorenck theorenck modified the milestones: 0.68.0, Short-term Jul 31, 2018
@crazy-max
Copy link
Contributor

Any news on this ?

@ChrissW-R1
Copy link
Contributor

Any news on this ?

Another PR which will never be merged?

@tassoevan tassoevan closed this Apr 29, 2019
@engelgabriel engelgabriel reopened this Aug 8, 2019
@engelgabriel engelgabriel modified the milestones: Short-term, 2.1.0 Aug 8, 2019
@engelgabriel
Copy link
Member

We will work to keep GIFs and PNGs as they are, convert SVGs to PNGs and everything else will be converted to JPG.

@engelgabriel engelgabriel modified the milestones: 2.1.0, 2.2.0 Oct 13, 2019
@rodrigok
Merge remote-tracking branch 'origin/develop' into fix.avatars-transp…
49b2c4e 
…arency

# Conflicts:
#	packages/rocketchat-file-upload/server/lib/FileUpload.js
@rodrigok
Allow SVG and GIF converting to PNG
5aebaf8
@rodrigok rodrigok changed the title [FIX] Transparency in avatars [NEW] Accept GIFs and SVGs for Avatars converting them to PNG and keep transparency of PNGs Oct 20, 2019
@rodrigok rodrigok merged commit 66f44ec into develop Oct 20, 2019
@rodrigok rodrigok deleted the fix.avatars-transparency branch October 20, 2019 21:12
@rodrigok rodrigok mentioned this pull request Oct 27, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigok rodrigok rodrigok approved these changes

@engelgabriel engelgabriel engelgabriel left review comments

@vynmera vynmera vynmera left review comments

@ggazzo ggazzo Awaiting requested review from ggazzo

Assignees
No one assigned
Labels
ui/ux
Projects
None yet
Milestone
2.2.0
10 participants
@tassoevan @rodrigok @rafaelks @gabrielsixel @crazy-max @ChrissW-R1 @engelgabriel @vynmera @theorenck @ggazzo