Reduced emoji spacing and increased emoji size

[mananjadhav]

@chiragsalian Can you please review this?

Details

Reduced spacing between emojis and increased emoji size.

Fixed Issues

$ #4544

Tests

1. Tested emoji picker on all platforms
2. Tested landscape mode. Added before and after screenshots.

QA Steps

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Portrait

Landscape

Desktop

iOS

Android

[chiragsalian]

With these changes to emoji sizes i see these as unused.
https://github.com/mananjadhav/App/blob/2a6e5b9323e2e8c6bd423a0c3d67169fe4415238/src/styles/styles.js#L1209-L1215

Could we remove them and iconSizeLarge?

Rest of your code looks good to me.

[mananjadhav]

@chiragsalian iconSizeLarge isn't used but iconSizeNormal, iconSizeSmall are used. I kept it as is just to retain the sequence. Do you still want me to remove it?

And the highlighted lines were still unused before my change. Let me know if you want me to remove those as well.

[shawnborton]

Can you help me understand why we need so many different sizes for emojis?

[mananjadhav]

@shawnborton We are currently using extraSmall, Normal and Large. I’ve added the small as well to ensure continuous range and if it gets used in the future.

The above three sizes are added so that the overall container is size is not increased. We use different sizes based on device width.

https://github.com/Expensify/App/blob/2a6e5b9323e2e8c6bd423a0c3d67169fe4415238/src/pages/home/report/EmojiPickerMenu/dynamicEmojiSize.js

[shawnborton]

So emojiSizeSmall is not used? If it's not used and we aren't sure if/when it will ever be used in the future, we can remove it.

That being said, I am questioning why we really need 3 sizes for this. For example, we only have one main text size that we share between mobile and desktop (15px). Can you try making this work with just one standard emoji size for the picker menu?

[mananjadhav]

Okay sure. I’ll remove the unused one.

I’ll try to make a common size. That would also mean either changing the container size or number of emojis per row. Is that something we are okay with ?

[shawnborton]

Why do you need to change the container size or number of emojis?

If you recall from the issue you recreated... I am not sure that this is a real problem we are solving. I'd say try just adjusting the spacing first, and let's see how that looks before modifying the emoji size. Thanks!

[mananjadhav]

Okay I’ll check.

I was following the existing structure where emojisizes are different based on device width. So are you suggesting we get rid of the current dynamicEmojiSize approach as well?

[shawnborton]

I think what I am saying is, let's just leave the emoji size alone to start (keep it how it currently is) and only modify the spacing of the buttons. Then let's review what the spacing changes do, and see if we need to make more adjustments from there.

[shawnborton]

I didn't realize we were currently using three different emoji sizes for the menu, and I personally think that's overkill.

[mananjadhav]

Yeah sorry for not being more clearer. Hence I highlighted the container size changes. So should I still go ahead and try to make the size consistent?

[shawnborton]

I would be curious to see what it looks like if we find one size that works everywhere... maybe it's something like 20?

[mananjadhav]

Sure lets try that. We can always rollback. I'll check and update you. 🚀

[shawnborton]

Awesome, thanks for your patience on this one!

[mananjadhav]

@shawnborton This is how it looks with fixed size 20, with less horizontal padding and desktop web container width reduced to 320 (earlier was 392). Let's take it forward from here?

All three views

Web/Desktop Emoji Container Size
Here's another comparison between Emoji Container size 320 and 352.

My take is fontSize 20 looks good on mobile landscape and portrait. Web fontSize 20 should be good with container size 352.

[shawnborton]

I think it looks pretty good. Thoughts?

[shawnborton]

And yeah, I think it's okay if we adjust the container size on desktop to make sure things look good there too.

[mananjadhav]

@shawnborton I agree, it looks good. For container on desktop, do prefer 320 or 352 ?

[shawnborton]

320 looks pretty good to me I think. Which do you prefer?

[mananjadhav]

Well I was included towards 352, but after checking a emoji mart, slack etc, it seems 320 might look better

[mananjadhav]

@chiragsalian I've updated the PR removing a fair amount of unused code. Can you please review it again?

[shawnborton]

Awesome, thanks again for listening to my feedback and working through our comments!

[mananjadhav]

@chiragsalian Can you please review this PR again with the updated changes?

[chiragsalian]

Nice, looks clean 👍

[OSBotify]

✋ 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]

🚀 Deployed to staging by @chiragsalian in version: 1.0.88-3 🚀

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

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀

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