-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: [Search v1] Search page scroll is not smooth #42170
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@eh2077 |
I have read the CLA Document and I hereby sign the CLA |
recheck |
@charles-liang Do you mean trying to open the search page on native Apps? If so, I think you can just follow the first reproduction step from the original issue
|
@charles-liang Can you handle the conflicts? |
@charles-liang And don’t worry if CI workflows are not running automatically. It needs help from @luacmartins to trigger it because it's your first PR and it's a little bit special. You can run |
I mean i run it in iOS |
I don't think it's necessary to edit
to run native apps? |
@charles-liang When opening a PR, please merge your branch to the |
I think the issue you encountered here is because you're not working on the |
@eh2077 I have completed the iOS and Android native screenshots. The conflicts have also been resolved and merged into the main branch. Thank you for tutoring me. |
windowSize={111} | ||
updateCellsBatchingPeriod={200} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the rationale for choosing these values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The windowSize
is set to 111 because each page can contain a maximum of 500 items, and this size is sufficiently large to ensure that the entire page will not be recycled.
The choice of 200 for updateCellsBatchingPeriod
is because 200ms out of 1000ms allows it to be slightly larger, preventing continuous loading while not causing too much delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use different values for desktop and mobile devices? As their screens size are different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recycling of cells is only necessary in cases such as phones with low memory. I tested in iPhone 14 pro is very smooth and not need recycling.
why I set it to 111. This size ensures that in both portrait mode and landscape mode, is large enough not to be recycled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we leave a comment explaining that please?
rightHandSideComponent={ | ||
rightHandSideComponent && | ||
(typeof rightHandSideComponent === 'function' | ||
? rightHandSideComponent({} as TItem) // Example usage of the function, replace with actual item | ||
: rightHandSideComponent) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's from the original code, and then when the GitHub Action runs typecheck, it throws a type error.
I'll continue testing when the site is full recovered. |
@charles-liang I'm checkout this branch locally and still can't open search page. |
Can you open the Chrome Developer Tools' Network tab to check the server response speed? I thought the network was slow because I am in China and there is a firewall. |
I also tested on the main branch and it was same - loading. But the staging env is working. So, I'm going to revisit this after the merging freeze. |
@eh2077 can you open the console and head to the |
@trjExpensify Oh, sorry I forgot to attach the screenshot of request. Yeah, the backend returned invalid data though the HTTP code is 200. |
So? what do I need to do? I feel like it's not an issue with my code. |
@charles-liang In this case, I think you can just wait until I'm able to re-test it and then provide feedback. Thanks. @trjExpensify Should we pull in another internal engineer (since @luacmartins is OOO) to take a look at the API issue, or should we just wait a little while? |
@eh2077 @charles-liang the pagination API is only in staging and we have a merge freeze in place. We'll need to wait a bit longer to get that deployed to production. Meanwhile, you should be able to test this using the staging API. Let me know if you still have issues with that. |
@eh2077@luacmartins Got it. I will pay attention to it in the future. |
… for Cell Component
updateCellsBatchingPeriod?: number; | ||
|
||
/** | ||
* The number passed here is a measurement unit where 1 is equivalent to your viewport height. The default value is 21 (10 viewports above, 10 below, and one in between). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value is 5 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was copied from the original documentation. It is correct. The default value of 5 was used in the previous project.
Reviewer Checklist
Screenshots/VideosAndroid: Native0-android.mp4Android: mWeb Chrome0-mobile-chrome.mp4iOS: Native0-ios.mp4iOS: mWeb Safari0-mobile-safari.mp4MacOS: Chrome / Safari0-web.mp4MacOS: Desktop0-desktop.mp4 |
@luacmartins This PR overall looks good to me and tested well. Just defer one comment (about fixing type error from main) to you. Kindly help to trigger the workflow to see if CI is green, thanks! |
Running workflow now. |
@eh2077 seems like typecheck is passing. |
<Icon | ||
src={Expensicons.ArrowRightLong} | ||
width={variables.iconSizeXXSmall} | ||
height={variables.iconSizeXXSmall} | ||
fill={theme.icon} | ||
/> | ||
<View style={[styles.flex1, styles.mw50]}>{userCell(transactionItem.to)}</View> | ||
<View style={[styles.mw50]}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.4.77-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
@eh2077
Details
#41581 [$250] [Search v1] Search page scroll is not smooth
Fixed Issues
$ #41581 (comment)
PROPOSAL: #41581 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
untitled.webm
Android: mWeb Chrome
android.mov
iOS: Native
ios-native.mov
iOS: mWeb Safari
ios.mov
MacOS: Chrome / Safari
2024-05-14.01.13.09.2.mov
2024-05-14.01.13.46.mov
MacOS: Desktop
desktop.mov
desktop.mov