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

Add avatars based on svg icons for default rooms #3891

Merged
merged 5 commits into from
Jul 10, 2021

Conversation

TomatoToaster
Copy link
Contributor

@TomatoToaster TomatoToaster commented Jul 6, 2021

@jasperhuangg, please review (also you worked on this recently, so you've got context!)
CC: @yuwenmemon, @shawnborton

Details

Adds the default room avatars s

Fixed Issues

Last part of this ticket, so we can close it out
$ https://github.com/Expensify/Expensify/issues/161777

Tests

Web QA done locally.

QA Steps

Since this is defaultRooms related, this needs to be QA'ed internally. I'll test this when this hits staging. Just ping @TomatoToaster, if this hasn't been checked off in QA checklist.

  1. Find default rooms and make sure their avatars match the screenshots.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

image
image
image
image

Also in focus mode:
image

@TomatoToaster TomatoToaster self-assigned this Jul 6, 2021
@shawnborton
Copy link
Contributor

I'm not entirely sure how you plan to implement this, but is it easier if we just provide the avatar image as an .svg at the proper 40x40 size with the correct background/foreground color included? I only ask because I do think it would be nice if that whole 40x40 image was just one asset that would scale nicely depending on situation, which might be:

Thoughts?

@TomatoToaster
Copy link
Contributor Author

TomatoToaster commented Jul 8, 2021

Just catching this @shawnborton. I think if we have just an svg file hosted somewhere similar to the other avatars here with the right colors it would be easy. I think regardless of the size, it would automatically scale like our current avatars do. The other avatars are 400x400 pngs so I believe regardless of the size of the svg it'll scale correctly right?

For more context how I planned to implement it right here would be to give the appropriate styling to the svgs so it looks like the mockup. Currently it looks like this:
image

@shawnborton
Copy link
Contributor

That's correct! Could you try just using this .svg for the room's avatar? This might make life way easier for you:
avatar-room.svg.zip

@TomatoToaster
Copy link
Contributor Author

Figured out how to apply styles to SVGs (doesn't work the same as <Image> it turns out!) and now this is looking cleaaan:
image

I'm going to make the code a little cleaner around how the large icons are displayed then this will be ready.

@shawnborton
Copy link
Contributor

Nice! And then how does this look when you tap on the room name in the chat header as well?

@TomatoToaster
Copy link
Contributor Author

Just updated the original body and added screenshots for the header, sidebar, details page version of the icons. What do you think @shawnborton?

@TomatoToaster TomatoToaster marked this pull request as ready for review July 8, 2021 20:26
@TomatoToaster TomatoToaster requested a review from a team as a code owner July 8, 2021 20:26
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team July 8, 2021 20:26
@shawnborton
Copy link
Contributor

Looks great! I would also add in a quick screenshot for what this looks like in #focus mode, but otherwise I think it's good to go - nice work!

@jasperhuangg
Copy link
Contributor

Yo @TomatoToaster!

The code looks good but for whatever reason when testing locally it seems like I'm getting the old avatars to show. Am I missing something?
Screen Shot 2021-07-09 at 8 19 26 PM

@TomatoToaster
Copy link
Contributor Author

Hmm now that is quite unusual. It seems like you're on an older commit of my branch. Can you delete my local branch and then re-pull it?

I actually deleted the old armchair icons so you shouldn't be able to render them. Can you see if theres an armchair.svg or an IconAvatar.js in the version of my branch you have locally?

@TomatoToaster
Copy link
Contributor Author

Merge conflicts! I fixed em 👍🏾

@jasperhuangg jasperhuangg merged commit 57f557c into main Jul 10, 2021
@jasperhuangg jasperhuangg deleted the amal-default-room-avatars branch July 10, 2021 05:22
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

5 participants