[No QA] [Performance] Approximate the ideal number of reports to render on first render#4371
[No QA] [Performance] Approximate the ideal number of reports to render on first render#4371marcaaron merged 3 commits intoExpensify:mainfrom rdjuric:calculateIdealNumOfMessages
Conversation
rdjuric
changed the title
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Looks good. Just had a few comments.
| {...props} | ||
| ref={ref} | ||
| initialNumToRender={20} | ||
| initialNumToRender={props.initialNumToRender} |
There was a problem hiding this comment.
Probably we can just remove this since each platform version is getting the prop passed via ...props same for the android, web, versions.
I'm not sure it should be a required prop as we can still fallback to the default value in cases where we are not getting more specific. Other things may use the inverted FlatList without this optimization.
There was a problem hiding this comment.
🤦♂️ Thanks for the catch! This really is unnecessary. Changed the code for all platforms.
| * Calculates the ideal number of reports to render in the first render, based on the screen height and on | ||
| * the height of the smallest report possible. |
There was a problem hiding this comment.
| * Calculates the ideal number of reports to render in the first render, based on the screen height and on | |
| * the height of the smallest report possible. | |
| * Calculates the ideal number of report actions to render in the first render, based on the screen height and on | |
| * the height of the smallest report action possible. |
| * @return {Number} | ||
| */ | ||
| calculateInitialNumToRender() { | ||
| const smallestReport = styles.chatItem.paddingTop + styles.chatItem.paddingBottom |
There was a problem hiding this comment.
name minimumReportActionHeight ?
| // Remove clipped subviews helps prevent avatars and attachments from eating up excess memory on Android. When | ||
| // we run out of memory images stop appearing without any warning. | ||
| // eslint-disable-next-line react/jsx-props-no-multi-spaces | ||
| shouldRemoveClippedSubviews |
There was a problem hiding this comment.
I don't think we want this here. Just removing the initialNumToRender={20}. IIRC removing the clipped sub views has unexpected effects on iOS (which is why there are separate files for iOS and Android).
| {...props} | ||
| ref={ref} | ||
| initialNumToRender={20} | ||
| initialNumToRender={props.initialNumToRender} |
There was a problem hiding this comment.
We can revert all these changes in this file I think?
|
Sorry @rdjuric just dawned on me that you maybe were not ready for another review 😓 |
|
No worries @marcaaron, I had forgot about Android 🥲 Should be ready for review now! |
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
This is working great for me thanks!
|
✋ 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 in version: 1.0.82-8🚀
|
|
@rdjuric Hello! Any QA tests needed for this one? |
|
I don't think there are any specific steps besides regressions around navigating to a chat |
marcaaron
changed the title
|
🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀
|
4 participants
Details
Calculated the ideal number of reports to render in the first render based on the screen height and the minimum height of a report
Fixed Issues
$ #4258
Tests
QA Steps
Tested On
Screenshots
Web
web.mov
Mobile Web
mweb.mp4
Desktop
desktop.mov
iOS
ios.mp4
Android
Android.mp4