Modify emojiHeaderContainer style with a -1 top, only on non-native devices

[patrickcze]

Details

As per the discussion on #15282 there is a browser specific bug leading to a small gab between the emoji category header and the header in the emoji list as you scroll. This is leading to a flickering effect as the emojis scroll behind the header and into this gap.

As the chrome team has opted not to fix this for a number of years the agreed upon interim solution is to apply a top:-1 space on non-native device.

Fixed Issues

$ #15282
$ #15282 (comment)

Tests

1. Load app
2. After log in
3. Open any chat
4. Click on emoji icon in bottom right of message bar
5. Scroll through list of emojis, confirming no gap is present between the emoji category picker and the emoji category headers as you scroll

• Verify that no errors appear in the JS console

Offline tests

n/a

QA Steps

1. Load app
2. After log in
3. Open any chat
4. Click on emoji icon in bottom right of message bar
5. Scroll through list of emojis, confirming no gap is present between the emoji category picker and the emoji category headers as you scroll

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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 is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• 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.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Chrome.MP4

Mobile Web - Safari

Safari.MP4

Desktop

Electron.-.New.Expensify.-.28.February.2023.mp4

iOS

Simulator.-.iPhone.14.Pro.Max.-.1.March.2023.mp4

Android

Android.webm

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[patrickcze]

@s77rt Thanks for your help with the comments on the Issue, i think the changes in this PR reflect what you were thinking. I am wondering if you could give any tips before continuing through the submission process.

I also have not been able to get android or iOS versions working locally. I noticed some links to stack overflow articles on how to get iOS and android working, but im not able to access them unfortunately since i dont have an @expensify.com email address.

Is there some other way to verify that this change does not affect iOS or Android native versions?

[MelvinBot]

@thienlnam @s77rt One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[patrickcze]

I have read the CLA Document and I hereby sign the CLA

[s77rt]

@patrickcze Thanks for the quick PR. I have few notes regarding the PR template:

• Do not alter the PR template (it could break automation)
  • "Verify that no errors appear in the JS console" checkbox is after the test steps
  • "Validate manual test in Chrome..." why did you add this?
• The tests are not clear enough, you have to be explicit, as a rule of thumb over-explain (Login, Go to any chat, etc.)
  • You missed the go to any chat step which is critical otherwise it's confusing.
  • The last step "no emoji flickering is experienced" is not correct, we are fixing the gap issue here not flicker effects.
• Do not include offline tests if they are not applicable i.e. there is no difference in the expected behaviour compared with "Tests". Just write n/a.

---

Running Android and iOS should work out of the box, just confirm you have the required dev env (Android Studio, Xcode, etc.)

• Android:
  1. npm run android
• iOS:
  1. cd ios; pod install; cd ..
  2. npm run ios

---

I will start review once you complete the author checklist, please tag me once you do.

[patrickcze]

Thanks for the feedback @s77rt, i have updated the template. Ill give android and iOS another shot

[patrickcze]

@s77rt Check list is complete, and Ready for Review. Do I need to wait for my proposal in UpWork being approved first?

Thanks again for your assistance on this one.

[s77rt]

@patrickcze Glad to see you got Android and iOS working. No you don't need to worry about Upwork for now. I will review asap.

@thienlnam The CLA test is failing, mind taking a look

[s77rt]

Code looks good, just few minor changes are needed

[thienlnam]

Hey @patrickcze just an FYI, all PRs go through a review process from on of our C+ and then a final review from an Expensify employee before it can be merged

[s77rt]

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:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • 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 is localized by adding it to src/languages/* files and using the translation method
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • 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 any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• 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.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

mweb-chrome.mp4

Mobile Web - Safari

mweb-safari.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

[s77rt]

LGTM! 🚀

cc @thienlnam

[thienlnam]

Awesome, thanks!

[MelvinBot]

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

[thienlnam]

Jest test 3 was taking multiple hours to run / the other two passed already

[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.

[github-actions]

Performance Comparison Report 📊

Significant Changes To Duration

Name	Duration
Open Search Page TTI	612.299 ms → 668.193 ms (+55.894 ms, +9.1%) 🔴

Show details

Name

Duration

Open Search Page TTI

Baseline Mean: 612.299 ms Stdev: 13.857 ms (2.3%) Runs: 587.1113689988852 594.1100260000676 594.6144210007042 594.9195959996432 596.0774739999324 598.2276200000197 599.7842209991068 602.8874510005116 604.0515140015632 604.093669001013 606.2131350003183 608.1176349986345 608.2720139995217 608.668335000053 609.0002440009266 611.19165099971 612.1845300011337 612.4694420006126 615.1818039994687 615.3343509994447 615.4839679989964 618.0861000008881 618.1299230009317 618.4188640005887 622.1642669998109 623.6664639990777 631.003866000101 632.2084550000727 634.9815680012107 639.6505939997733 644.9634599983692 Current Mean: 668.193 ms Stdev: 20.303 ms (3.0%) Runs: 614.4752599988133 623.6779379993677 645.8032229989767 648.1492110006511 650.4184980001301 653.2187099996954 653.422322999686 653.8327639997005 656.83158400096 658.0545660015196 659.8020419999957 661.9793299995363 662.3183999992907 663.6454270016402 664.7486570011824 666.1603190004826 669.8827720005065 671.5011800006032 672.6694339998066 675.3274750001729 676.1014000009745 676.465942999348 677.3994550015777 683.9759520012885 684.9093019999564 685.8529870007187 688.5332440007478 689.3091230001301 691.4235030002892 693.3111579995602 693.5970869995654 715.3926999997348

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	700.660 ms → 726.577 ms (+25.916 ms, +3.7%)
App start runJsBundle	189.452 ms → 199.710 ms (+10.258 ms, +5.4%)
App start nativeLaunch	19.344 ms → 20.067 ms (+0.723 ms, +3.7%)
App start regularAppStart	0.014 ms → 0.014 ms (-0.000 ms, -1.7%)

Show details

Name	Duration
App start TTI	Baseline Mean: 700.660 ms Stdev: 31.323 ms (4.5%) Runs: 623.9865210000426 658.994434999302 666.4706079997122 672.8555250000209 672.9342509992421 674.217630000785 676.15693599917 677.7130330000073 679.0244410000741 679.9985739998519 681.0631990004331 690.8380539994687 691.275504000485 693.5162979997694 695.3043389990926 697.8565410003066 701.9131310004741 703.1044880002737 706.9812119994313 707.6520879995078 712.7381929997355 713.8066809996963 715.60252700001 716.0176960006356 716.5693999994546 732.5408069994301 735.8541689999402 745.4602170009166 746.2668389994651 751.4975429996848 782.2574380002916 Current Mean: 726.577 ms Stdev: 36.987 ms (5.1%) Runs: 673.4980529993773 679.4592129997909 682.849274000153 686.1251240000129 688.5138339996338 693.3840650003403 694.8212749995291 695.3282379992306 697.006895000115 700.2082979995757 705.5716500002891 706.7107679992914 709.2376940008253 714.152710000053 715.3400159999728 716.1072269994766 718.5194210000336 718.7600020002574 721.0077689997852 734.6172449998558 735.0954890009016 736.5413610003889 740.9950900003314 750.7913380004466 755.0633419994265 758.7222089990973 761.5728309992701 772.503575000912 773.4419790003449 787.5256089996547 804.2062389999628 822.7772199995816
App start runJsBundle	Baseline Mean: 189.452 ms Stdev: 19.745 ms (10.4%) Runs: 159 163 168 169 170 171 174 174 177 179 180 183 183 184 184 184 185 186 186 188 189 192 198 206 207 211 214 217 224 225 243 Current Mean: 199.710 ms Stdev: 21.634 ms (10.8%) Runs: 175 175 177 178 180 181 181 183 183 183 185 185 185 190 191 194 195 195 202 202 207 209 209 216 216 218 225 232 240 242 257
App start nativeLaunch	Baseline Mean: 19.344 ms Stdev: 1.994 ms (10.3%) Runs: 17 17 18 18 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 21 21 21 21 22 22 25 25 Current Mean: 20.067 ms Stdev: 1.788 ms (8.9%) Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 21 22 22 23 24 25
App start regularAppStart	Baseline Mean: 0.014 ms Stdev: 0.001 ms (6.3%) Runs: 0.012411000207066536 0.012816999107599258 0.012980001047253609 0.012980001047253609 0.01302100159227848 0.01314299926161766 0.013265000656247139 0.013345999643206596 0.013387000188231468 0.01342799887061119 0.013671999797224998 0.013753999024629593 0.013833999633789062 0.013875000178813934 0.013956001028418541 0.013957001268863678 0.014038000255823135 0.014078998938202858 0.014567000791430473 0.01464799977838993 0.014688998460769653 0.014689000323414803 0.014730000868439674 0.014770999550819397 0.014810999855399132 0.014892999082803726 0.015095999464392662 0.015095999464392662 0.015177000313997269 0.01534000039100647 0.015421999618411064 0.01603199914097786 Current Mean: 0.014 ms Stdev: 0.000 ms (3.3%) Runs: 0.01285799965262413 0.012899000197649002 0.013183999806642532 0.013387000188231468 0.013387000188231468 0.01342800073325634 0.013590000569820404 0.013671999797224998 0.013753000646829605 0.013753000646829605 0.013875000178813934 0.013875000178813934 0.013875000178813934 0.014078998938202858 0.014079000800848007 0.01411999948322773 0.01411999948322773 0.014120001345872879 0.014159999787807465 0.014159999787807465 0.014161000028252602 0.014161000028252602 0.014201000332832336 0.014322999864816666 0.014444999396800995 0.014526000246405602 0.01464799977838993

[github-actions]

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

[OSBotify]

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.2.78-0 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀

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