Issue 6390 - Fixes emoji positioning by sidferreira · Pull Request #6766 · Expensify/App

sidferreira · 2021-12-15T01:08:59Z

Details

This issue addresses some positioning issues for emojis in iOS.
Some changes affects other platforms, but minor changes.

Fixed Issues

$ #6390

Tests

QA Steps

To test: App/src/pages/home/HeaderView.js

Needs a default room, with a subtitle.
Look at the room subtitle.
Emoji should be fully visible, no positioning change on other platforms.
Adding/removing the emoji should not change the text position.

To test: App/src/pages/ReportDetailsPage.js

Needs a default room, with a subtitle.
Go to the room details.
Emoji should be fully visible, the component had height: 16px; paddingBottom: 24px; now it has height: 20px; paddingBottom: 20px;.
The text is basically ~4px lower with this.
Adding/removing the emoji should not change the text position.

To test: App/src/pages/home/sidebar/OptionRow.js

Go to a chat, send an emoji
Go back to chat list
Emoji should be fully visible, the description component had height: 16px; paddingTop: 4px; now it has height: 20px; paddingTop: 0px;.
The text moves a ~1px up with the change.
Adding/removing the emoji should not change the text position.

Tested On

Screenshots

App/src/pages/home/HeaderView.js

Platform	Before	After
App iOS
App Android
Mobile Safari
Mobile Chrome
Desktop
Web Chrome
Web Safari

App/src/pages/ReportDetailsPage.js

Platform	Before	After
App iOS
App Android
Mobile Safari
Mobile Chrome
Desktop
Web Chrome
Web Safari

App/src/pages/home/sidebar/OptionRow.js `Most Recents`

Platform	Before	After
App iOS
App Android
Mobile Safari
Mobile Chrome
Desktop
Web Chrome
Web Safari

App/src/pages/home/sidebar/OptionRow.js `Focus` (avoid regression)

Platform	Before	After
App iOS
App Android
Mobile Safari
Mobile Chrome
Desktop
Web Chrome
Web Safari

parasharrajat · 2021-12-15T12:07:39Z

src/styles/expensiTextInputPlatformStyles/index.ios.js

+export default {
+    paddingTop: 19,
+};


From #6390 (comment).
We should try to avoid using styles specific to platforms. Try to generalize the styles so that they look good on each platform.

This change affects only IOS creating inconsistency across platforms. Also, this is a big design decision and thus I will ask someone to review this.

The default padding top is 23, I'll move it to the file for clarity. @parasharrajat

Also, I had to do similar in another issue related to emojis

I agree, I forgot to mention that at that time but there, UI was not affected. The text was vertically aligned on all platforms and here the gap is not equal.

Can you help me understand what this change does, and how it helps us fix the issue that is being addressed here?

@parasharrajat gotcha

@shawnborton long story short, emojis on iOS display a bit larger ( #6390 (comment) ) and require some extra pixels to display it properly. The input has height: 100%; paddingBottom: 8; paddingTop: 23, and the parent has height: 50, so, the usable height for the input is 19, but as the input has fontSize: 15, it needs 4 extra pixels to render the emoji properly, that's why changing paddingTop: 23 to paddingTop: 19 address the issue.

We can also use paddingBottom: 6; paddingTop: 21 to try to keep it a bit more at center...

parasharrajat · 2021-12-15T12:08:18Z

src/styles/styles.js

    expensiTextInputAndIconContainer: {
        zIndex: -1,
        flexDirection: 'row',
+        height: '100%',


This could be breaking Multiline input. Could you test it and share a clip of that with me?

@parasharrajat I tested Web Safari and App iOS and it was alright. I'll run screenshots once we settle the requested changes...

parasharrajat · 2021-12-15T12:09:30Z

@shawnborton This PR is changing the padding for TextInputs across the app to paddingTop: 19px. Could you please check the screenshots and tell us if this is fine?

shawnborton · 2021-12-15T16:05:03Z

I don't understand why the text input components are being touched when this issue is about fixing how emojis display in the chats list. Can someone help me understand? Thanks!

parasharrajat · 2021-12-15T16:13:46Z

@shawnborton This also affects the TextInput and Emojis are cropped on TextInput in IOS too. #6390 (comment).

Scope was extended.

shawnborton · 2021-12-15T16:16:40Z

Just to clarify, how did that scope get added here? Seems like that should be a separate project?

shawnborton · 2021-12-15T16:18:16Z

Okay looks like it was discussed to group these together in the issue, cc @roryabraham

shawnborton · 2021-12-15T16:20:44Z

src/styles/styles.js

    optionAlternateText: {
-        height: 16,
-        lineHeight: 16,
+        height: 20,


I'm not sure how I feel about this change. Right now, we have the user name/email set to be 20px tall, then a 4px margin, then the alternate text is 16px. This gives us 20 + 4 + 16 = 40px, which sits perfectly next to the 40px tall avatar. Can you make sure we get this same kind of evenness with these proposed changes?

@shawnborton the margin was removed, as discussed in #6390 (comment)

@sidferreira could you please expand on Shawn's comment. We want to be sure from a design perspective.

Quoting from the link:

To fix this, we need to tweak optionAlternateText in 3 places:

Use case 1

App/src/pages/home/HeaderView.js

Lines 151 to 159 in ecce012

<ExpensifyText

style={[

styles.sidebarLinkText,

styles.optionAlternateText,

styles.textLabelSupporting,

styles.mt1,

]}

numberOfLines={1}

>

Use case 2

App/src/pages/home/sidebar/OptionRow.js

Line 95 in 0311b18

: [textStyle, styles.optionAlternateText, styles.textLabelSupporting, styles.mt1];

Have a total height 20 as the component height is 16 and mt1 is 4.

Use case 3

App/src/pages/ReportDetailsPage.js

Lines 147 to 155 in ecce012

<ExpensifyText

style={[

styles.sidebarLinkText,

styles.optionAlternateText,

styles.textLabelSupporting,

styles.mb6,

]}

numberOfLines={1}

>

Has a total height 40 as the component height is 16 and mt6 is 24

So, my suggestion is to:
A) change optionAlternateText to have lineHeight/height: 20, behaving kinda like mt1 by default
B) Remove mt1 from use cases 1 and 2
C) Change mb6 to mb5 in use case 3

This change will make sure that all items keep their designed heights but allow space to emoji wherever is needed.

sidferreira · 2021-12-15T16:53:52Z

Just to clarify, how did that scope get added here? Seems like that should be a separate project?

@shawnborton yeah, @parasharrajat and I wondered if we should split it or not, but sounded smaller than turned into...

parasharrajat · 2021-12-15T17:01:41Z

Well, as mentioned "Well, I am not yet sure of the best solution to fix it. I would have to dig into the code for that. But I would say that this is additional to the main issue so we can still proceed with the main issue and fix all the know issues and think about the input later in detail. I will report the issue separately and then we can explore there."

I think it's better to split the Textinput issue into a separate one. So I am reporting it on Slack.
we can proceed with the rest of the issues.
is it fine? @roryabraham @sidferreira

This reverts commit ee5b660.

sidferreira · 2021-12-15T17:07:17Z

@parasharrajat @roryabraham @shawnborton revered input changes

roryabraham

This all seems fine to me, but let's see what @shawnborton thinks

parasharrajat · 2021-12-16T16:24:46Z

Awaiting Shawn's input before final review.

shawnborton · 2021-12-16T21:05:57Z

I think this looks good to me, thanks!

OSBotify · 2021-12-16T21:35:51Z

✋ 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 · 2021-12-17T17:57:57Z

🚀 Deployed to staging by @roryabraham in version: 1.1.21-2 🚀

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

mvtglobally · 2021-12-17T22:29:33Z

@roryabraham @sidferreira @parasharrajat How can we create default room, with a subtitle?
When we select new room There is no "subtitle" option.

sidferreira · 2021-12-17T23:22:12Z

@mvtglobally for development purposes I forced those cases, I'm not sure how to create it as well... It seems to require admin rights or something...

roryabraham · 2021-12-21T14:51:51Z

@mvtglobally since:

That's not yet a mainline use-case
@sidferreira verified that case on dev
All the others are a pass

I think that gives me enough confidence to check this off and run the deploy.

OSBotify · 2021-12-21T19:44:49Z

🚀 Deployed to production by @sketchydroide in version: 1.1.22-0 🚀

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

sidferreira added 2 commits December 14, 2021 16:36

Emoji fixes on iOS - optionAlternateText

595f6c7

Emoji fixes on iOS - Inputs

ee5b660

sidferreira marked this pull request as ready for review December 15, 2021 01:51

sidferreira requested a review from a team as a code owner December 15, 2021 01:51

botify requested review from parasharrajat and roryabraham December 15, 2021 01:51

MelvinBot removed the request for review from a team December 15, 2021 01:51

sidferreira changed the title ~~Sidferreira/issue 6390~~ Issue 6390 - Fixes emoji positioning Dec 15, 2021

sidferreira mentioned this pull request Dec 15, 2021

[HOLD for payment 2021-12-28] iOS - Emoji top is cut in LHN message preview #6390

Closed

parasharrajat suggested changes Dec 15, 2021

View reviewed changes

shawnborton reviewed Dec 15, 2021

View reviewed changes

Revert "Emoji fixes on iOS - Inputs"

8925786

This reverts commit ee5b660.

roryabraham approved these changes Dec 16, 2021

View reviewed changes

shawnborton approved these changes Dec 16, 2021

View reviewed changes

roryabraham requested a review from parasharrajat December 16, 2021 21:34

roryabraham merged commit f6b139c into Expensify:main Dec 16, 2021

sidferreira deleted the sidferreira/issue_6390 branch December 16, 2021 21:38

OSBotify mentioned this pull request Dec 17, 2021

Deploy Checklist: New Expensify 2021-12-17 #6815

Closed

31 tasks

	<ExpensifyText
	style={[
	styles.sidebarLinkText,
	styles.optionAlternateText,
	styles.textLabelSupporting,
	styles.mt1,
	]}
	numberOfLines={1}
	>

Conversation

sidferreira commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

To test: App/src/pages/home/HeaderView.js

To test: App/src/pages/ReportDetailsPage.js

To test: App/src/pages/home/sidebar/OptionRow.js

Tested On

Screenshots

App/src/pages/home/HeaderView.js

App/src/pages/ReportDetailsPage.js

App/src/pages/home/sidebar/OptionRow.js Most Recents

App/src/pages/home/sidebar/OptionRow.js Focus (avoid regression)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Dec 15, 2021

Uh oh!

shawnborton commented Dec 15, 2021

Uh oh!

parasharrajat commented Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawnborton commented Dec 15, 2021

Uh oh!

shawnborton commented Dec 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidferreira commented Dec 15, 2021

Uh oh!

parasharrajat commented Dec 15, 2021

Uh oh!

sidferreira commented Dec 15, 2021

Uh oh!

roryabraham left a comment

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Dec 16, 2021

Uh oh!

shawnborton commented Dec 16, 2021

Uh oh!

OSBotify commented Dec 16, 2021

Uh oh!

OSBotify commented Dec 17, 2021

Uh oh!

mvtglobally commented Dec 17, 2021

Uh oh!

sidferreira commented Dec 17, 2021

Uh oh!

roryabraham commented Dec 21, 2021

Uh oh!

OSBotify commented Dec 21, 2021

sidferreira commented Dec 15, 2021 •

edited

Loading

App/src/pages/home/sidebar/OptionRow.js `Most Recents`

App/src/pages/home/sidebar/OptionRow.js `Focus` (avoid regression)

parasharrajat Dec 15, 2021 •

edited

Loading

parasharrajat commented Dec 15, 2021 •

edited

Loading