perf: reduce rerenders of sidebar by hannojg · Pull Request #12369 · Expensify/App

hannojg · 2022-11-02T12:13:28Z

Details

There was this PR for improving the performance by reducing the rerenders of the sidebar, which is a heavy component.

perf: memo SidebarLinks #11907

However, this PR introduced a bug where the FAB wouldn't show on android anymore.

This PR includes the previous PR + a fix to make sure it's working on android.
The main solution was to omit the behavior prop.

This PR also fixed another issue, where on tablets there was a great empty area when opening the keyboard:

Fixed Issues

$ #12119
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Click the + FAB on the HomeScreen
Press the emoji picker in a chat
Press the "attachment" icon in the chat composer
Verify that its possible to open and close all these modals flawlessly without any jank (You should e.g. see the animation on the FAB button)

Verify that no errors appear in the JS console

QA Steps

Same as testing steps!

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

This reverts commit 830ee42.

hannojg · 2022-11-02T13:18:32Z

/cc @Julesssss

mountiny

The changes look good to me, just one probably forgotten style. Great and quick catch @hannojg I have added @tgolen for a review since he might have deeper context about what the padding behaviour prop has been used for and if there should be any sideffects not obvious to us by removing it.

src/styles/styles.js

tgolen · 2022-11-02T15:30:29Z

I have added @tgolen for a review since he might have deeper context about what the padding behaviour prop has been used for and if there should be any sideffects not obvious to us by removing it.

When I was working on these components, I found that "experimenting" was the best approach to finding what worked for the behavior prop. The official docs say that it's a best practice to include the behavior prop for both Android and iOS:

Android and iOS both interact with this prop differently. On both iOS and Android, setting behavior is recommended.

https://reactnative.dev/docs/keyboardavoidingview

So, I think it might be a mistake to omit the prop entirely. Is there a different behavior, like using "position" or "height" which will fix the problem, and also not lead to a regression on other platforms? Please be sure to carefully test this change on all platforms.

Santhosh-Sellavel · 2022-11-02T20:30:33Z

@hannojg This PR will be in conflict, We are gonna remove wrapping inside KeyboardAvoidingView in this PR #12345

Also the PR is also towards fixing empty area when opening the keyboard in iPad/Tablet

cc: @tgolen

hannojg · 2022-11-02T21:17:52Z

@Santhosh-Sellavel ah yeah great, then let's wait for that to get merged first? Just wondering, if we remove the KAV then we won't have the behaviour that on tablets when opening the keyboard, the FAB will float above the keyboard, no?

// EDIT: just seen in the PR's screenshots that's the functionality is preserved, great!

hannojg · 2022-11-02T21:19:02Z

Also added the behaviour prop back with height which creates the desired result for every platform (as far as my tests tell me)

Santhosh-Sellavel · 2022-11-02T21:21:21Z

@hannojg Please have a look at the attachments in the PR description and feel free to test the PR, it floats above the keyboards except for one edge case I pointed out here

and tgolen's remark for that here

Santhosh-Sellavel · 2022-11-09T00:13:04Z

@hannojg This can come off hold, please resolve conflicts!

mountiny

Code looks good to me as it is mostly coming from previous PR.

@hannojg @Santhosh-Sellavel Can you please specifically test for this issue #12281 and repro steps mentioned there? thank you very much!

…f/memoize-sidebar-2

hannojg · 2022-11-09T10:17:24Z

(conflicts resolved, good to get tested again!)

tgolen · 2022-11-09T16:03:00Z

src/pages/home/sidebar/SidebarScreen/PopoverModal.js

+        allPolicies: {
+            key: ONYXKEYS.COLLECTION.POLICY,
+        },


I don't think it should be done in this PR (this PR is already old enough), but I think it would be great to implement the Onyx property selectors here, like was done in sidebarlinks:

https://github.com/Expensify/App/blob/main/src/pages/home/sidebar/SidebarLinks.js#L221-L224

That could really help with performance here.

mountiny

Agree with @tgolen, I think follow up issue should handle that. This is already a re-revert

deetergp · 2022-11-09T19:52:27Z

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.

deetergp

Code looks good, but I've got a couple requests:

Can you be more explicit about the QA steps. Are they the same as the testing steps? If so, then plainly state it.
Since this hasn't been deployed to staging, then how could you have tested whether or not there were errors in the JS console? Please un-check the QA box.

src/pages/home/sidebar/SidebarScreen/PopoverModal.js

mountiny · 2022-11-17T20:12:15Z

@hannojg There is a Jest Unit test job failing, I tried to restart it but it failed again. I guess this must be related to this PR too

hannojg · 2022-11-17T20:14:10Z

@mountiny hm no, I think that one is failing on the main branch as well, and only happened since I merged main branch into this branch some minutes ago 😅

mountiny · 2022-11-17T20:15:20Z

@hannojg Just came here to say I see it on other PR too, wondering when this started.

hannojg · 2022-11-17T20:16:22Z

There is already a fix on the way. not sure if seen already 😊 #12831

mountiny · 2022-11-17T20:20:17Z

Nice, yeah so we can merge this once that is merged and Santosh posts the checklist.

mountiny · 2022-11-17T20:22:04Z

#12831 merged, waiting for santosh only 🙌

…f/memoize-sidebar-2

…-app-fork into perf/memoize-sidebar-2

mountiny · 2022-11-18T11:19:12Z

@Santhosh-Sellavel Lets get this merged today! Great job on this one @hannojg and Santosh for catching the edge cases.

Santhosh-Sellavel · 2022-11-18T16:42:07Z

On it

Santhosh-Sellavel · 2022-11-18T16:43:25Z

Reviewer Checklist

Screenshots/Videos

Desktop & Web

Screen.Recording.2022-11-18.at.11.05.50.PM.mov

Mobile Web - Chrome

Screen_Recording_20221118_230116_Chrome.mp4

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.14.-.2022-11-18.at.23.03.05.mp4

iOS & Android

Screen.Recording.2022-11-18.at.10.55.13.PM.mov

Santhosh-Sellavel

All looks good, tests well!

cc: @mountiny @hannojg let's get this 🚢 !

melvin-bot · 2022-11-21T01:10:02Z

@mountiny 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.

mountiny · 2022-11-21T01:11:23Z

Well, how to say it Melvin, I think you heard it many times by now. The tests were passing, stop gaslighting me!

OSBotify · 2022-11-22T16:36:38Z

🚀 Deployed to staging by @mountiny in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-22T17:46:23Z

🚀 Deployed to staging by @mountiny in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-24T00:04:45Z

🚀 Deployed to production by @luacmartins in version: 1.2.30-0 🚀

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

mountiny · 2022-12-14T21:37:33Z

Noting per the checklist that this PR has introduced a regression #13159, we will add a new regression test so this is caught in staging if it would happen again. The reason for regression has been a mistake during refactoring, copy/pasting and we did not catch this in a review.

mollfpr · 2023-06-29T02:39:55Z

src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js

+
+    componentDidMount() {
+        const routes = lodashGet(this.props.navigation.getState(), 'routes', []);
+        Welcome.show({routes, showCreateMenu: this.showCreateMenu});


✋ Coming from #20609

The FAB button shows up on the Abracadabra page after opening the main browser tab and going back to the Abracadabra page.

For the fix, we conditionally show the popover only on the CENTRAL_PANE_NAVIGATOR or the home page.

hannojg added 2 commits November 2, 2022 10:03

Revert "revert PR 11907 which introduced missing FAB bug"

242aca5

This reverts commit 830ee42.

fix floating action button not showing

ee41772

hannojg requested a review from a team as a code owner November 2, 2022 12:13

melvin-bot bot requested review from Santhosh-Sellavel and deetergp and removed request for a team November 2, 2022 12:13

hannojg mentioned this pull request Nov 2, 2022

[CP Staging] Revert PR 11907 which introduced missing FAB bug #12336

Merged

90 tasks

Julesssss assigned hannojg Nov 2, 2022

Julesssss mentioned this pull request Nov 2, 2022

perf: memo SidebarLinks #11907

Merged

92 tasks

mountiny requested a review from tgolen November 2, 2022 13:51

mountiny reviewed Nov 2, 2022

View reviewed changes

src/styles/styles.js Outdated Show resolved Hide resolved

hannojg added 2 commits November 2, 2022 21:13

remove unused style

ef9a878

use height behavior

bd49a71

mountiny changed the title ~~perf: reduce rerenders of sidebar~~ [HOLD PR#12345] perf: reduce rerenders of sidebar Nov 3, 2022

mountiny changed the title ~~[HOLD PR#12345] perf: reduce rerenders of sidebar~~ perf: reduce rerenders of sidebar Nov 9, 2022

mountiny reviewed Nov 9, 2022

View reviewed changes

Merge branch 'main' of github.com:margelo/expensify-app-fork into per…

9ef93a7

…f/memoize-sidebar-2

tgolen previously approved these changes Nov 9, 2022

View reviewed changes

mountiny previously approved these changes Nov 9, 2022

View reviewed changes

deetergp requested changes Nov 9, 2022

View reviewed changes

Santhosh-Sellavel suggested changes Nov 10, 2022

View reviewed changes

src/pages/home/sidebar/SidebarScreen/PopoverModal.js Outdated Show resolved Hide resolved

hannojg added 2 commits November 17, 2022 21:27

Merge branch 'main' of github.com:margelo/expensify-app-fork into per…

633f803

…f/memoize-sidebar-2

Merge branch 'perf/memoize-sidebar-2' of github.com:margelo/expensify…

a140dc6

…-app-fork into perf/memoize-sidebar-2

mountiny approved these changes Nov 17, 2022

View reviewed changes

deetergp approved these changes Nov 17, 2022

View reviewed changes

tgolen approved these changes Nov 18, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Nov 18, 2022

View reviewed changes

mountiny merged commit 3528ad5 into Expensify:main Nov 21, 2022

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

mountiny removed the Emergency label Nov 21, 2022

hannojg mentioned this pull request Nov 21, 2022

[$1000] [Performance] Pressing the FAB on the start screen doesn't open the menu immediately #12119

Closed

5 tasks

OSBotify mentioned this pull request Nov 22, 2022

Deploy Checklist: New Expensify 2022-11-22 #12934

Closed

40 tasks

This was referenced Nov 29, 2022

[Hold for payment 2022-12-20] Web - The "FAB" menu and the sidebar can be opened at the same time #13159

Closed

fix "FAB menu and sidebar can be opened at the same time" #13226

Merged

mollfpr reviewed Jun 29, 2023

View reviewed changes

mollfpr mentioned this pull request Jun 29, 2023

[HOLD for payment 2023-06-28] [$1000] FAB menu is displayed abracadabra page when creating a new account #20609

Closed

6 tasks

Conversation

hannojg commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Uh oh!

hannojg commented Nov 2, 2022

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgolen commented Nov 2, 2022

Uh oh!

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

Uh oh!

Uh oh!

hannojg commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannojg commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Santhosh-Sellavel commented Nov 9, 2022

Uh oh!

mountiny left a comment • 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!

hannojg commented Nov 9, 2022

Uh oh!

tgolen Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

PR Reviewer Checklist

Uh oh!

deetergp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mountiny commented Nov 17, 2022

Uh oh!

hannojg commented Nov 17, 2022

Uh oh!

mountiny commented Nov 17, 2022

Uh oh!

hannojg commented Nov 17, 2022

Uh oh!

mountiny commented Nov 17, 2022

Uh oh!

mountiny commented Nov 17, 2022

Uh oh!

mountiny commented Nov 18, 2022

Uh oh!

Santhosh-Sellavel commented Nov 18, 2022

Uh oh!

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

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

hannojg commented Nov 2, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 2, 2022 •

edited

Loading

hannojg commented Nov 2, 2022 •

edited

Loading

hannojg commented Nov 2, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 2, 2022 •

edited

Loading

mountiny left a comment •

edited

Loading

deetergp commented Nov 9, 2022 •

edited

Loading

Santhosh-Sellavel commented Nov 18, 2022 •

edited

Loading

mountiny commented Nov 21, 2022 •

edited

Loading