App: [HOLD for payment 2023-02-23] [$1000] Figure out why `svg` images don't show immediately when uploading as avatar

@trjExpensify I tested extensively with many different SVG files, including the file used in https://github.com/Expensify/App/issues/15189 and couldn’t reproduce the issue.

Any thoughts @Beamanator @Santhosh-Sellavel ?

okay unassigning myself then and looking for other bugs

Ah shit, that was the weekend. Yes, you’re right! I’ve paid you $500 as a bonus.

@Ollyws Aah I see 👍 I think that default file name is only used if somehow the App can’t access the uploaded filename. So if we can do the same here that would be great 👍

And yes please add svg to that array of constants 👍 (otherwise we won’t be able to select svgs to upload 😅 )

Cool then we won’t need that name check then we could hardcode a fixed name straight away.

Looks good to me, and works well.

const imageName = isSvg ? ‘fileName.png’ : props.imageName;

@Beamanator Are we fine with this hardcoded file name, do we need to do something about it? Ex: replace extension in name

@Ollyws I’m using your latest diff, that’s nice that that fixes the black background issue 👍 👍

I think it’s fine if the SVG conversion happens locally 👍

@Santhosh-Sellavel let me know what you think about @Ollyws 's latest proposal here, I think it’s pretty solid 👍

@Beamanator Yes exactly.

I believe we are converting the SVG to a PNG/JPEG locally using canvas.toBlob then uploading the resulting PNG/JPEG blob to the server, so the SVG conversion is all happening locally.

Which diff are you using where you aren’t seeing the black background? In my latest update the black background shouldn’t occur.

Will review tomorrow!

@PankajAS I updated the OP to include that step

@JmillsExpensify @trjExpensify ok I made this new issue for bmp and gif avatar images here: https://github.com/Expensify/App/issues/14751

For this issue, I think we can make this specifically be for .svg avatar images, update the title & OP, then open externally

Nice, I think you probably have the most context for the issues to create with the sleuthing to date and what we’re after upstream, so probably clearer if you go ahead and create those. 👍

@JmillsExpensify I’m 99% sure there’s nothing blocking default avatars here, just the uploading of new custom SVG avatars by users

I think it’s

1. I am under the impression that Old Dot does not support SVGs for avatars, and user-uploaded avatars are referenced from the same key in the user’s personal details on both Old and NewDot. So if you uploaded an SVG in NewDot (say we store the function), it could work, but then your avatar would be broken when you log back into Old Dot.
2. Yes
3. Yes, the way that we were handling default avatars in New Dot was by hashing logins. So right now, it technically doesn’t matter if you have a path to your default avatar saved or not as long as we know the login. Therefore, we were able to store all the default avatar SVGs locally and just grab the correct one whenever we hashing the login. I was following the patterns we already had in place while implementing that PR specifically. For instance, how we handle the SVG workspace avatars.

I’m a bit confused trying to piece this together:

1. we’re saying that NewDot has a problem with storing SVG avatars locally
2. Members can’t upload SVG avatars because of (1)
3. We’re able to use SVG avatars in NewDot for the default avatars despite the limitation in (1)?
4. To do (2) we need to add support for SVG avatars in OldDot as well, that’s a concern because of ??

We’d probably benefit from writing up a summary of where we’re at with these moving parts, adding that to the OP, and starting a discussion on whatever decisions we need to make to go from her.

@Beamanator @trjExpensify We should move the WAQ hold in my opinion in any case.

https://github.com/Expensify/App/issues/13910 should be fixed or made not an issue by this GH when it’s done, closing the other one.

While this issue currently exists, I would prefer waiting till https://github.com/Expensify/App/pull/12549 gets merged before working on this (most likely can be external) since in that PR we’re enabling other image formats to be uploaded