Prevent emoji Picker list jumping while scrolling through arrow keys by Pujan92 · Pull Request #13787 · Expensify/App

Pujan92 · 2022-12-22T01:50:19Z

Details

Emoji picker menu height changes to display it properly and taking advantage of flatlist getItemLayout for fixed height optimization

Fixed Issues

$ #12772
PROPOSAL: #12772 (comment)

Tests

Open any chat
Click on the emoji picker button
Navigate the emoji list with arrow key
Verify selected/highlighted emoji is fully visible in the view

Verify that no errors appear in the JS console

Offline tests

QA Steps

Open any chat
Click on the emoji picker button
Navigate the emoji list with arrow key
Verify selected/highlighted emoji is fully visible in the view

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

Mobile Web - Safari

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.webm

…vantage of flatlist getItemLayout for fixed height optimization

melvin-bot · 2022-12-22T01:50:52Z

@Gonals Please 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]

Santhosh-Sellavel

Mostly looks good, just some refactoring!

Santhosh-Sellavel · 2022-12-22T03:32:55Z

src/components/EmojiPicker/EmojiPickerMenu/index.js

+                            getItemLayout={(data, index) => (
+                                {length: CONST.EMOJI_PICKER_ITEM_HEIGHT, offset: CONST.EMOJI_PICKER_ITEM_HEIGHT * index, index}
+                            )}


Please move this into a separate function and like renderItem

Santhosh-Sellavel · 2022-12-22T22:25:07Z

src/components/EmojiPicker/EmojiPickerMenu/index.js

+    /**
+     * @param {*} data
+     * @param {Number} index
+     * @returns {Object}
+     */
+    getItemLayout(data, index) {
+        return {length: CONST.EMOJI_PICKER_ITEM_HEIGHT, offset: CONST.EMOJI_PICKER_ITEM_HEIGHT * index, index};
+    }
+


Can you quickly add comments for the params and a brief comments for the method also?

Santhosh-Sellavel · 2022-12-22T22:36:44Z

Test step 4

List selected index should not jump in the view

Could be changed to "Verify selected/highlighted emoji is fully visible in the view"

Santhosh-Sellavel · 2022-12-22T22:37:13Z

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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Web_Emoji.mov

Mobile Web - Chrome & Mobile Web - Safari

Screen.Recording.2022-12-23.at.9.50.48.PM.mov

Desktop

Screen.Recording.2022-12-23.at.3.58.40.AM.mov

iOS

Screen.Recording.2022-12-23.at.4.02.40.AM.mov

Android

Screen.Recording.2022-12-23.at.4.00.53.AM.mov

Santhosh-Sellavel · 2022-12-22T22:39:32Z

@Pujan92 Issue should be linked as

$ https://github.com/Expensify/App/issues/12772 will display as $ #12772

Pujan92 · 2022-12-23T02:20:56Z

@Pujan92 Issue should be linked as

$ https://github.com/Expensify/App/issues/12772 will display as $ #12772

Ok, got it. Thanks. 👍

Santhosh-Sellavel

LGTM tests well. All you @Gonals or @Luke9389

cc: @JmillsExpensify

JmillsExpensify · 2022-12-23T20:26:46Z

Both Luke and Alberto are out today as a heads up. It looks like Luke is back on Monday, so let's re-assess where things are at then.

Gonals

LGTM too!
@Luke9389 all yours!

JmillsExpensify · 2022-12-27T04:34:05Z

Do we have to wait on @Luke9389 at this point? Now that at least one of you has approved, and so has C+, I say we go with that and merge this PR.

Pujan92 · 2022-12-27T16:40:22Z

Do we have to wait on @Luke9389 at this point! Now that at least one of you has approved, and so has C+, I say we go with that and merge this PR.

Are we fine to merge this or need to wait for @Luke9389 ?

JmillsExpensify · 2022-12-27T17:44:32Z

Ooops, that was also meant to be a question on my part. @Gonals What do you think?

Luke9389

Looks great, thanks

Luke9389 · 2022-12-27T22:26:21Z

Hey y'all. I've been OOO. PR looks good. Once the Reviewer checklist gets fixed feel free to merge w/o me.

Santhosh-Sellavel · 2022-12-28T00:47:39Z

I don't see any issues with the checklist let's merge this one @Gonals

OSBotify · 2022-12-28T10:03:34Z

✋ 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 · 2022-12-28T10:22:36Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	842.014 ms → 851.421 ms (+9.407 ms, +1.1%)
nativeLaunch	9.419 ms → 9.677 ms (+0.258 ms, +2.7%)
regularAppStart	0.015 ms → 0.015 ms (+0.000 ms, +3.0%)
runJsBundle	196.813 ms → 193.867 ms (-2.946 ms, -1.5%)

Show details

Name	Duration
TTI	Baseline Mean: 842.014 ms Stdev: 46.232 ms (5.5%) Runs: 743.9859130000696 758.0354899996892 776.214328000322 781.2805160004646 790.4590410003439 793.2844709996134 814.4164570001885 820.6053449995816 820.8042019996792 823.1821929998696 824.4548960002139 826.1023869998753 830.1521509997547 830.8355860002339 832.0048850001767 832.7073739999905 836.5758509999141 843.6072509996593 849.3056239997968 856.7335609998554 869.1439869999886 870.8779490003362 872.3545009996742 873.2277899999171 875.4770149998367 883.8814249997959 900.8488299995661 903.7998730000108 917.1944310003892 922.3204239998013 928.552283000201 Current Mean: 851.421 ms Stdev: 38.049 ms (4.5%) Runs: 771.8828079998493 796.7778650000691 802.9781160000712 812.9604489998892 814.4038079995662 819.5439109997824 827.0988499997184 828.850421000272 830.2686059996486 831.2237339997664 831.4623109996319 835.6348489997908 837.8388350000605 841.3127530002967 842.1483380002901 843.9650349998847 845.2405460001901 847.7281440002844 848.551130999811 853.2233100002632 860.2920509995893 860.7861489998177 867.951306999661 880.5881279995665 883.8692669998854 897.1839870000258 899.549354000017 903.809236000292 913.6363159995526 928.5459500001743 934.7472219998017
nativeLaunch	Baseline Mean: 9.419 ms Stdev: 1.681 ms (17.8%) Runs: 7 7 7 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 10 10 10 10 11 11 11 11 11 12 13 14 Current Mean: 9.677 ms Stdev: 1.468 ms (15.2%) Runs: 7 8 8 8 8 8 8 8 8 9 9 9 9 10 10 10 10 10 10 10 10 10 11 11 11 11 11 11 11 13 13
regularAppStart	Baseline Mean: 0.015 ms Stdev: 0.001 ms (7.1%) Runs: 0.012492000125348568 0.012655000202357769 0.013387000188231468 0.013549999333918095 0.013590999878942966 0.013672000728547573 0.014038000255823135 0.014038000255823135 0.014078999869525433 0.014078999869525433 0.014281999319791794 0.014282000251114368 0.01444500032812357 0.01448499970138073 0.014566999860107899 0.014688999392092228 0.014689999632537365 0.0147299999371171 0.014933000318706036 0.014973999932408333 0.015014000236988068 0.015056000091135502 0.015178000554442406 0.015381000004708767 0.015584999695420265 0.015625 0.015665999613702297 0.015910000540316105 0.01603100076317787 0.016316000372171402 0.016479999758303165 0.017170999199151993 Current Mean: 0.015 ms Stdev: 0.001 ms (6.1%) Runs: 0.01355000026524067 0.013671999797224998 0.0139979999512434 0.01403799932450056 0.014119000174105167 0.014363999478518963 0.014364000409841537 0.014403999783098698 0.01444500032812357 0.014526000246405602 0.014607999473810196 0.014689000323414803 0.014771000482141972 0.014851000159978867 0.015014000236988068 0.015014000236988068 0.015137000009417534 0.015177999623119831 0.015259000472724438 0.015339999459683895 0.01534000039100647 0.015461999922990799 0.015461999922990799 0.01590999960899353 0.015949999913573265 0.016032000072300434 0.01607299968600273 0.0163569999858737 0.016439000144600868 0.016439000144600868 0.016886000521481037 0.017292999662458897
runJsBundle	Baseline Mean: 196.813 ms Stdev: 23.588 ms (12.0%) Runs: 165 166 167 169 169 172 174 174 175 176 177 181 182 188 192 196 197 200 200 204 211 212 212 212 216 217 220 222 227 239 241 245 Current Mean: 193.867 ms Stdev: 15.463 ms (8.0%) Runs: 169 170 170 173 173 175 178 185 186 187 189 189 191 192 192 192 195 198 201 202 203 204 205 206 208 208 210 213 226 226

OSBotify · 2022-12-28T20:02:52Z

🚀 Deployed to staging by @Gonals in version: 1.2.45-0 🚀

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

JmillsExpensify · 2022-12-28T20:34:44Z

Thanks @Luke9389 and welcome back!

OSBotify · 2022-12-29T19:41:17Z

🚀 Deployed to production by @chiragsalian in version: 1.2.45-0 🚀

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

thesahindia · 2023-02-06T19:04:03Z

This PR caused a regression (#14462). More context.

Santhosh-Sellavel · 2023-02-06T19:24:44Z

Oops missed a minor detail!

emoji picker menu height changes to display it properly and taking ad…

d60002c

…vantage of flatlist getItemLayout for fixed height optimization

Pujan92 requested a review from a team as a code owner December 22, 2022 01:50

melvin-bot bot requested review from Gonals and removed request for a team December 22, 2022 01:50

Santhosh-Sellavel suggested changes Dec 22, 2022

View reviewed changes

Pujan92 added 2 commits December 22, 2022 09:58

getItemLayout function added

3e7db85

lint fix

733adc3

Pujan92 requested review from Santhosh-Sellavel and removed request for Gonals December 22, 2022 04:41

Santhosh-Sellavel reviewed Dec 22, 2022

View reviewed changes

comment added

ffd0201

Pujan92 requested a review from Santhosh-Sellavel December 23, 2022 02:22

Santhosh-Sellavel approved these changes Dec 23, 2022

View reviewed changes

Gonals requested review from Gonals and Luke9389 December 26, 2022 09:50

Gonals approved these changes Dec 26, 2022

View reviewed changes

Luke9389 approved these changes Dec 27, 2022

View reviewed changes

Gonals merged commit 36b50c1 into Expensify:main Dec 28, 2022

OSBotify mentioned this pull request Dec 28, 2022

Deploy Checklist: New Expensify 2022-12-28 #13868

Closed

6 tasks

melvin-bot bot mentioned this pull request Dec 29, 2022

[HOLD for payment 2023-01-05] [$4000] Emoji Picker list jumps while scrolling through arrow keys reported by @parasharrajat #12772

Closed

stitesExpensify mentioned this pull request Jan 25, 2023

Add Emoji Category Picker #14532

Merged

54 tasks

daraksha-dk mentioned this pull request Jan 25, 2023

[HOLD for payment 2023-02-06] [$1000] The emoji titles are not vertically centered in android #14462

Closed

2 tasks

Conversation

Pujan92 commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Dec 22, 2022

Uh oh!

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

Pujan92 Dec 23, 2022

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel commented Dec 22, 2022

Uh oh!

Santhosh-Sellavel commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

Santhosh-Sellavel commented Dec 22, 2022

Uh oh!

Pujan92 commented Dec 23, 2022

Uh oh!

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Uh oh!

JmillsExpensify commented Dec 23, 2022

Uh oh!

Gonals left a comment

Choose a reason for hiding this comment

Uh oh!

JmillsExpensify commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pujan92 commented Dec 27, 2022

Uh oh!

JmillsExpensify commented Dec 27, 2022

Uh oh!

Luke9389 left a comment

Choose a reason for hiding this comment

Uh oh!

Luke9389 commented Dec 27, 2022

Uh oh!

Santhosh-Sellavel commented Dec 28, 2022

Uh oh!

OSBotify commented Dec 28, 2022

Uh oh!

github-actions bot commented Dec 28, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

Uh oh!

OSBotify commented Dec 28, 2022

Uh oh!

JmillsExpensify commented Dec 28, 2022

Uh oh!

OSBotify commented Dec 29, 2022

Uh oh!

thesahindia commented Feb 6, 2023

Uh oh!

Santhosh-Sellavel commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Pujan92 commented Dec 22, 2022 •

edited

Loading

Santhosh-Sellavel commented Dec 22, 2022 •

edited

Loading

JmillsExpensify commented Dec 27, 2022 •

edited

Loading