Fix Desktop - CMD + K is slow and laggy #3601 #3760

Looxor · 2021-06-25T18:32:32Z

Details

src/pages/SearchPage.js

added filterAdapter
added getCustomHeaderMessage
removed onChangeText
added conditional render using didScreenTransitionEnd

src/components/OptionsSelector.js

removed props.onChangeText
added filterAdapter
added getCustomHeaderMessage

Fixed Issues

$ #3601

Tests

While using e.cash use the cmd+k command
Type on the search field

QA Steps

While using e.cash use the cmd+k command
Type on the search field

Tested On

Screenshots

Web

Mobile Web

Desktop

expensify-fix-result.mp4

iOS

Android

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

github-actions · 2021-06-25T18:32:49Z

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

Looxor · 2021-06-28T14:38:19Z

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

thienlnam · 2021-06-28T16:14:27Z

src/components/OptionsSelector.js

+    filterAdapter: PropTypes.func,
+    getCustomHeaderMessage: PropTypes.func,


Each prop needs a comment

thienlnam · 2021-06-28T16:17:49Z

src/components/OptionsSelector.js

@@ -207,12 +214,12 @@ class OptionsSelector extends Component {
                    ref={el => this.list = el}
                    optionHoveredStyle={styles.hoveredComponentBG}
                    onSelectRow={this.selectRow}
-                    sections={this.props.sections}
+                    sections={this.state.sections || this.props.sections}


Can we just initialize the state with the sections from props?

thienlnam · 2021-06-28T16:18:46Z

src/components/OptionsSelector.js

                    focusedIndex={this.state.focusedIndex}
                    selectedOptions={this.props.selectedOptions}
                    canSelectMultipleOptions={this.props.canSelectMultipleOptions}
                    hideSectionHeaders={this.props.hideSectionHeaders}
-                    headerMessage={this.props.headerMessage}
+                    headerMessage={headerMessage}


don't need a separate variable for this, let's just put this.props.getCustomHeaderMessage(this.searchValue) here

thienlnam · 2021-06-28T16:21:14Z

src/pages/SearchPage.js

@@ -62,11 +59,10 @@ class SearchPage extends Component {
    constructor(props) {
        super(props);

-        Timing.start(CONST.TIMING.SEARCH_RENDER);


We still want the Timing I believe, can we get this added back but maybe put where it belongs now?

thienlnam · 2021-06-28T16:21:45Z

src/pages/SearchPage.js

+        // eslint-disable-next-line no-console
+        console.log('lazyLoad', this.state.readyToLoad);


Don't still need this log line

thienlnam · 2021-06-28T16:44:41Z

src/pages/SearchPage.js

+                        ? (
+                            <Text>
+                                ...
+                            </Text>
+                        )


We definitely don't want to show the ... text when the screen is loading, we should either show a loading indicator here or just leave it like how it was before

I removed it.

I removed log again.

I updated everything you asked.

Looxor · 2021-06-28T18:43:48Z

I updated everything as you asked.

thienlnam · 2021-06-28T18:46:15Z

src/components/Looxor

+Looxor
+MyNameIsJin08011952l.
+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


Don't think you want this file in here

I removed it.
It was made by my mistake when I create a token file by reading a tutorial of my friend. Sorry.

thienlnam · 2021-06-28T18:46:57Z

src/components/OptionsSelector.js

+    /** Function to get a filtered result as list */
+    filterAdapter: PropTypes.func,
+    /** FUnction to get header message from parent component */
+    getCustomHeaderMessage: PropTypes.func,


New line before each comment, also a typo with FUnction

Yes, I added them.

thienlnam · 2021-06-28T18:48:10Z

src/pages/SearchPage.js

+                    !didScreenTransitionEnd
+                        ? null
+                        : (


Should just make this {({didScreenTransitionEnd}) => ( and remove the null conditional

Yes, I removed it.

thienlnam · 2021-06-28T18:49:24Z

src/pages/SearchPage.js

    }

    componentDidMount() {
-        Timing.end(CONST.TIMING.SEARCH_RENDER);


The timing isn't getting ended so this will just be an infinite timer

Yes, I added it. Sorry for forgetting.

thienlnam · 2021-06-28T23:18:41Z

src/components/OptionsSelector.js

+    /** Function to get a filtered result as list */
+    filterAdapter: PropTypes.func,
+
+    /** FUnction to get header message from parent component */


Typo with FUnction

Yes, updated. Sorry for typo

thienlnam · 2021-06-28T23:24:46Z

src/pages/SearchPage.js

+        this.recentReports = recentReports;
+        this.personalDetails = personalDetails;
+        this.userToInvite = userToInvite;


Won't these need to be left in state so this can re-render with changes to these values?

I didn't know that the SearchPage needs to re-render when updating props, but just found Onyx component wrapper.
Ok, I will change that to use this.props. (recentReports, personDetails, userToInvite)

thienlnam · 2021-06-28T23:28:33Z

src/pages/SearchPage.js

+     * @param {String} searchValue
+     * @returns {Array}
+     */
+    filterAdapter(searchValue) {


Confused at why this is called filterAdapter

Regarding this, honestly, I was also confused because I am not sure which type of design pattern is preferred in this project.
I renamed it to searchSections.
But, if it is not good, can you please recommend one?

thienlnam · 2021-06-28T23:29:14Z

src/pages/SearchPage.js

     * @returns {Array}
     */
    getSections() {
+        return this.filterAdapter('');
+    }


I think you need to rename this method / update the comment now

I renamed it to searchSections.

Looxor · 2021-06-29T08:09:18Z

Thank you for your time to fix my mistakes on updating codes.

…t tyop

thienlnam

Awesome, thanks for making all these changes! It's looking good

OSBotify · 2021-06-29T18:22:38Z

✋ 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-06-29T19:25:59Z

🚀 Deployed to staging in version: 1.0.74-1🚀

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

parasharrajat

This PR has caused a regression #3818.

parasharrajat · 2021-06-30T17:16:55Z

src/pages/SearchPage.js

+                                <OptionsSelector
+                                    sections={sections}
+                                    onSelectRow={this.selectReport}
+                                    searchSections={this.searchSections}


This prop needs to be passed to other pages where this component is used. Otherwise, Search will not work there.

So, do you mean to use this.props.sections?
It makes multiple-rendering as I explained here.
#3601 (comment)

parasharrajat · 2021-06-30T17:17:11Z

src/pages/SearchPage.js

-                                hideSectionHeaders
-                                hideAdditionalOptionStates
-                                showTitleTooltip
+                    didScreenTransitionEnd && (


Why didScreenTransitionEnd was moved up? I mean we didn't want to hide the header of the page behind the loader.

When I put didScreenTransitionEnd, I found that CMD+K is much faster.
But, if needed, I will put it back.

Fix Desktop - CMD + K is slow and laggy Expensify#3601

2d74cbc

Looxor requested a review from a team as a code owner June 25, 2021 18:32

MelvinBot requested review from thienlnam and removed request for a team June 25, 2021 18:32

thienlnam requested changes Jun 28, 2021

View reviewed changes

Looxor added 3 commits June 28, 2021 19:24

Remove unnecessary text on SearchPage

bdc8dad

Remove unnecessary log

9b13f15

Add Timing, Update sections, Add commments

141592a

Remove unnecessary variable, add lines before comments

2a18f5c

thienlnam requested changes Jun 28, 2021

View reviewed changes

Rename filterAdapter, Add Timing.end, Add props.recentReports, Correc…

4fe5bbf

…t tyop

thienlnam approved these changes Jun 29, 2021

View reviewed changes

thienlnam merged commit 7299d6c into Expensify:main Jun 29, 2021

OSBotify mentioned this pull request Jun 29, 2021

Deploy Checklist: Expensify.cash 2021-06-29 #3802

Closed

22 tasks

This was referenced Jun 30, 2021

Search for participant is not working #3818

Closed

[CP Staging] Revert "Fix Desktop - CMD + K is slow and laggy #3601" #3824

Merged

parasharrajat reviewed Jun 30, 2021

View reviewed changes

nickmurray47 mentioned this pull request Jul 15, 2021

Fix Search Page Slow Issue (CMD+K) - 2nd, 3rd solution #3980

Closed

5 tasks

		filterAdapter: PropTypes.func,
		getCustomHeaderMessage: PropTypes.func,

		// eslint-disable-next-line no-console
		console.log('lazyLoad', this.state.readyToLoad);

Fix Desktop - CMD + K is slow and laggy #3601 #3760

Fix Desktop - CMD + K is slow and laggy #3601 #3760

Conversation

Looxor commented Jun 25, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Jun 25, 2021 • edited Loading

Looxor commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Looxor commented Jun 28, 2021

Choose a reason for hiding this comment

Looxor Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Looxor commented Jun 29, 2021

thienlnam left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 29, 2021

OSBotify commented Jun 29, 2021

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Looxor commented Jun 25, 2021 •

edited

Loading

github-actions bot commented Jun 25, 2021 •

edited

Loading

Looxor Jun 29, 2021 •

edited

Loading

parasharrajat left a comment •

edited

Loading