Fix emoji offset issues by sobitneupane · Pull Request #8902 · Expensify/App

sobitneupane · 2022-05-09T14:25:05Z

Details

Fixed Issues

$ #8459

Tests

Open Emoji Picker from Report Action Compose Box.
Press down arrow key and arrow up key to navigate across emojis

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Open Emoji Picker from Report Action Compose Box.
Press down arrow key and arrow up key to navigate across emojis

Verify that no errors appear in the JS console

Screenshots

Web

Screen.Recording.2022-11-09.at.07.40.26.mov

Mobile Web - Chrome

screen_recording_chrome.mov

Mobile Web - Safari

Screen.Recording.2022-11-09.at.07.36.16.mov

Desktop

Screen.Recording.2022-11-09.at.07.43.10.mov

iOS

Screen.Recording.2022-11-09.at.07.33.29.mov

Android

Screen.Recording.2022-11-09.at.07.49.13.mov

parasharrajat

Ongoing testing...

parasharrajat · 2022-05-10T20:04:24Z

src/CONST.js

-    NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 300,
-    EMOJI_PICKER_ITEM_HEIGHT: 40,
+    NON_NATIVE_EMOJI_PICKER_LIST_HEIGHT: 295,
+    EMOJI_PICKER_ITEM_HEIGHT: 34.5,


We definitely don't want floats so let's round this off to 35.

I tried rounding off. But with 35, highlighted emoji shifts up with each click and with 34, shifts down with each click.

parasharrajat · 2022-05-12T18:01:17Z

src/components/EmojiPicker/EmojiPickerMenu/index.js

+        } else if (offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT <= this.currentScrollOffset) {
+            targetOffset = offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT;


Could you please explain this change?

As Emoji Picker Header is what remains at the top, we need to subtract emoji picker header height not emoji height. Here, this calculation is trying to detect if emoji has gone out of view while pressing up arrow.

let's put this as a comment in the code.

@parasharrajat Any suggestion?

} else if (offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT <= this.currentScrollOffset) { + // As Emoji Picker Header is what remains at the top, EMOJI_PICKER_HEADER_HEIGHT is subtracted from offsetAtEmojiTop targetOffset = offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT; }

parasharrajat

It's not working for me on Chrome Mac. After a while, highlighted emoji is out of view. Also, it is important that the fix works for reverse scrolling as well.

sobitneupane · 2022-05-16T12:37:11Z

It's not working for me on Chrome Mac. After a while, highlighted emoji is out of view.

I will take a look.

Also, it is important that the fix works for reverse scrolling as well.

I have tested it for reverse scrolling as well and it is working fine for reverse as well.

sobitneupane · 2022-05-22T14:14:49Z

Hello @parasharrajat! As you mentioned, after scrolling a while, the scrollbar bumps up and highlighted emoji goes out of view. So, I thought it might be because of initial number of emojis rendered in the flat list. I tried adding initialNumToRender. And it worked.

Also, I found that some emojis has different height. So, we need to add lineheight in emojiText in styles.js

    // Emoji Picker Styles
    emojiText: {
        fontFamily: fontFamily.GTA_BOLD,
        textAlign: 'center',
        fontSize: variables.emojiSize,
+        lineHeight: 27, //variables.emojiLineHeight
        ...spacing.pv0,
        ...spacing.ph0,
    },

We definitely don't want floats so let's round this off to 35.

After this change, we can avoid use of float in EMOJI_PICKER_ITEM_HEIGHT in CONST.js .

EMOJI_PICKER_ITEM_HEIGHT: 35,

parasharrajat · 2022-05-23T16:20:25Z

Ok.

I thought it might be because of initial number of emojis rendered in the flat list.

Could you please explain to me, how?

Also, I found that some emojis has different height. So, we need to add lineheight in emojiText in styles.js

Good point I was going to suggest the same. We should fix the height of emojis. But why line-height? Can't we do with height and width?

sobitneupane · 2022-05-26T12:30:55Z

I thought it might be because of initial number of emojis rendered in the flat list.

Could you please explain to me, how?

After scrolling a while, the scrollbar jumps up as if new items are added to the bottom of the scrollview and highlighted emoji goes out of view. So, I thought it might be because of initial number of items rendered. It should not be the case by default. But as we are using our own calculations to move the scroll view, this issue might have arisen.

Good point I was going to suggest the same. We should fix the height of emojis. But why line-height? Can't we do with height and width?

Yes, we can also achieve it by adding height in emojiItem.

    emojiItem: {
        width: '12.5%',
        textAlign: 'center',
        borderRadius: 8,
+       height: CONST.EMOJI_PICKER_ITEM_HEIGHT,
    },

But with this change, we need to make some change in styling in EmojiSkinToneList as the addition of height breaks the styling.

parasharrajat · 2022-05-26T16:21:47Z

After scrolling a while, the scrollbar jumps up as if new items are added to the bottom of the scrollview and highlighted emoji goes out of view.

we can maintain the scroll position.

Yes, we can also achieve it by adding height in emoji items.

Using line-Height will not work as expected all the time. Height on the other hand should be best for performance reasons as well. I think all emojis have fixed sizes so we should be good to use heights. Let's do that and you can also use line heights to center the emojis.

parasharrajat · 2022-06-05T20:02:46Z

Thanks for waiting, I will review this asap.

parasharrajat

Scrolling seems to work better now but there is one issue which you mentioned. I noticed such kind of issue when I was scrolling around Objects section.

parasharrajat · 2022-06-10T14:46:23Z

src/components/EmojiPicker/EmojiPickerMenu/index.js

                    : (
                        <FlatList
                            ref={el => this.emojiList = el}
+                            initialNumToRender={400}


This needs to be removed. This is not a solution to the problem you were mentioning.
First, I do not understand the problem. Please add a video so that we can open this to broader audiences.

This change will also hurt performance.

I noticed a similar issue. When I was scrolling around the objects section of emoji, highlighted emoji was out of view. I think this is due to something else. Let's try maintainescrollPosition.

With the introduction of intialNumToRender={400} (even we can have initialNumToRender as low as 100), this issue is no longer reproducible.

Screen.Recording.2022-06-16.at.19.11.16.mov

scrollPosition is maintained onScroll={e => this.currentScrollOffset = e.nativeEvent.contentOffset.y}. But this value this.currentScrollOffset changes abruptly resulting the issue. I cannot find any bug in calculations in scrollToHighlightedIndex() function. Introduction of initialNumToRender={100} solving the issue also suggests no bugs in scrollToHighlightedIndex()

But this value this.currentScrollOffset changes abruptly resulting the issue.

Good catch, we are one step closer to the real problem now. Now the thing is to find out what is causing this .

I cannot find the reason why this.currentScrollOffset value changes abruptly. I also cannot find the reason why, with the introduction of initalNumToRender, this.currentScrollOffset value does not change abruptly.

No luck. The flat list is giving me nuts.

So first let's set this to 100.

Change made.

parasharrajat · 2022-06-10T14:47:57Z

src/components/EmojiPicker/EmojiSkinToneList.js

                                () => this.setState(prev => ({isSkinToneListVisible: !prev.isSkinToneListVisible}))
                            }
                            style={[
-                                styles.pv1,


Let's add this back to the emoji but include this on the emoji height so that we can cover this value. Motive should always be to keep the same UI. I see that UI is little bit different from staging.

This is not the reason for slight change in UI. It is due to the introduction of EMOJI_PICKER_ITEM_HEIGHT and variables.emojiLineHeight.

parasharrajat · 2022-06-10T14:50:34Z

After scrolling a while, the scrollbar jumps up as if new items are added to the bottom of the scrollview and highlighted emoji goes out of view. So, I thought it might be because of initial number of items rendered. It should not be the case by default. But as we are using our own calculations to move the scroll view, this issue might have arisen.

I am also not sure what is causing this. But I think we need to maintain the scroll position to fix this.

parasharrajat · 2022-06-23T17:45:54Z

@sobitneupane Any update for me.

parasharrajat · 2022-08-08T17:47:52Z

@sobitneupane Could you please merge into this?

parasharrajat · 2022-08-31T11:30:41Z

Bump @sobitneupane.

sobitneupane · 2022-09-01T11:28:07Z

@sobitneupane Could you please merge into this?

I will do it shortly. I was away last two months. Sorry for the inconvenience.

parasharrajat · 2022-09-04T18:44:59Z

Bump.

parasharrajat · 2022-09-29T15:02:27Z

@sobitneupane I tested that there are no changes in the behavior after merging the latest main. But I am still actively looking into it. Could you please merge main again?

sobitneupane · 2022-09-29T15:24:53Z

@sobitneupane I tested that there are no changes in the behavior after merging the latest main. But I am still actively looking into it. Could you please merge main again?

@parasharrajat Merged Main.

parasharrajat · 2022-09-29T16:11:50Z

Thanks

parasharrajat · 2022-11-01T21:35:10Z

Bump @sobitneupane ⬆️

trjExpensify · 2022-11-03T21:22:03Z

@sobitneupane can you confirm when you've made changes and it's ready for re-review?

sobitneupane · 2022-11-03T21:24:27Z

It is ready for review.

trjExpensify · 2022-11-03T21:34:38Z

Cool, thanks! It would be great to comment to let people know explicitly, or request a re-review.

parasharrajat · 2022-11-04T14:06:30Z

I am facing some issues with Mac so I won't be able to test all platforms until that issue is fixed.

Hoping to get it fixed asap and get back on it.

Thanks

parasharrajat · 2022-11-07T18:03:49Z

@sobitneupane Could you please merge into this? Need that for testing.

sobitneupane · 2022-11-08T02:54:42Z

@sobitneupane Could you please merge into this? Need that for testing.

@parasharrajat Merged.

parasharrajat

Screenshots

🔲 iOS / native

screen-2022-11-09_04.29.01.mp4

🔲 iOS / Safari

screen-2022-11-09_03.23.10.mp4

🔲 MacOS / Desktop

screen-2022-11-09_03.21.53.mp4

🔲 MacOS / Chrome

screen-2022-11-09_00.35.27.mp4

screen-2022-11-09_00.38.24.mp4

🔲 Android / Chrome

screen-2022-11-09_02.47.49.mp4

🔲 Android / native

screen-2022-11-09_02.42.02.mp4

parasharrajat · 2022-11-08T23:01:36Z

It seems that emojis are not centered in the highlighted area on iOS. @sobitneupane Can you please update the videos in the details with the latest.

sobitneupane · 2022-11-08T23:57:37Z

Can you please update the videos in the details with the latest.

@parasharrajat Videos/Screenshots in the details are latest.

parasharrajat · 2022-11-09T00:04:08Z

It seems that emojis are not centered in the highlighted area on iOS.
ok, let's look into this.

trjExpensify · 2022-11-09T01:12:31Z

Oh, I have a feeling that's because of this issue that @fedirjh has just submitted a proposal.

Edit: Ah, you're not talking about the compose box once selected here. My mind when right there when looking at the video. 😅

sobitneupane · 2022-11-09T02:24:11Z

It seems that emojis are not centered in the highlighted area on iOS.
let's look into this.

@parasharrajat Issue fixed

parasharrajat

LGTM. Thanks for the changes and your patience.

cc: @chiragsalian

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

🎀 👀 🎀 C+ reviewed

chiragsalian

Code LGTM and seems to work well for me. Tested on web and ios.
Thanks for pushing this forward gents 🙇

parasharrajat · 2022-11-14T20:38:22Z

I will create a follow-up issue for the Flatlist behavior when this is merged.

melvin-bot · 2022-11-14T21:33:48Z

@chiragsalian looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

chiragsalian · 2022-11-14T21:34:33Z

Not an emergency, GH actions were failing on PR Reviewer Checklist but it looked fine to me so I merged it to move this ahead.

OSBotify · 2022-11-14T21:37:56Z

✋ 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 · 2022-11-15T21:58:29Z

🚀 Deployed to staging by @chiragsalian in version: 1.2.28-0 🚀

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

OSBotify · 2022-11-15T23:40:29Z

🚀 Deployed to staging by @chiragsalian in version: 1.2.28-0 🚀

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

OSBotify · 2022-11-17T07:37:31Z

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

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

Fix emoji offset issues

9c35a84

sobitneupane marked this pull request as ready for review May 10, 2022 09:04

sobitneupane requested a review from a team as a code owner May 10, 2022 09:04

melvin-bot bot requested review from chiragsalian and parasharrajat and removed request for a team May 10, 2022 09:04

parasharrajat suggested changes May 12, 2022

View reviewed changes

parasharrajat suggested changes May 13, 2022

View reviewed changes

Fix the height of emoji

0d9d205

sobitneupane requested a review from parasharrajat June 2, 2022 10:23

parasharrajat suggested changes Jun 10, 2022

View reviewed changes

Merge branch 'main' into sn_bug-emoji-positioning

8ea2fff

parasharrajat mentioned this pull request Jun 30, 2022

[HOLD for payment 2022-12-08] [HOLD for payment 2022-11-24] [$500] Highlighted emoji not visible after scrolling down with ArrowDown key after sometime - reported by @sobitneupane #8459

Closed

Merge main resolving conflict in EmojiSkinToneList

eb49313

Merge branch 'main' into sn_bug-emoji-positioning

98f45b7

Change emoji height

7a226a6

Merge branch 'main' into sn_bug-emoji-positioning

5ca7584

parasharrajat reviewed Nov 8, 2022

View reviewed changes

sobitneupane added 2 commits November 9, 2022 07:55

Align emoji center in IOS

18d826f

Fix lint error

450146f

parasharrajat approved these changes Nov 11, 2022

View reviewed changes

chiragsalian approved these changes Nov 14, 2022

View reviewed changes

chiragsalian merged commit d091ab6 into Expensify:main Nov 14, 2022

melvin-bot bot added the Emergency label Nov 14, 2022

chiragsalian removed the Emergency label Nov 14, 2022

OSBotify mentioned this pull request Nov 15, 2022

Deploy Checklist: New Expensify 2022-11-15 #12753

Closed

31 tasks

		} else if (offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT <= this.currentScrollOffset) {
		targetOffset = offsetAtEmojiTop - CONST.EMOJI_PICKER_HEADER_HEIGHT;

Conversation

sobitneupane commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

parasharrajat left a comment

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!

Choose a reason for hiding this comment

Uh oh!

sobitneupane Nov 2, 2022 • 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!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

sobitneupane commented May 16, 2022

Uh oh!

sobitneupane commented May 22, 2022

Uh oh!

parasharrajat commented May 23, 2022

Uh oh!

sobitneupane commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parasharrajat commented May 26, 2022

Uh oh!

parasharrajat commented Jun 5, 2022

Uh oh!

parasharrajat left a comment

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!

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 Jun 10, 2022

Uh oh!

parasharrajat commented Jun 23, 2022

Uh oh!

parasharrajat commented Aug 8, 2022

Uh oh!

parasharrajat commented Aug 31, 2022

Uh oh!

sobitneupane commented May 9, 2022 •

edited

Loading

sobitneupane Nov 2, 2022 •

edited

Loading

sobitneupane commented May 26, 2022 •

edited

Loading

parasharrajat commented Sep 29, 2022 •

edited

Loading

parasharrajat commented Nov 7, 2022 •

edited

Loading

trjExpensify commented Nov 9, 2022 •

edited

Loading