Show a skeleton view above optimistic report actions when we're still loading the rest of the report #13590

yuwenmemon · 2022-12-14T19:26:23Z

Details

If we're loading report actions and we know that the report has other actions that are going to be loaded, show a skeleton view above any optimistic report actions so that we can convey the fact we're loading more actions

Fixed Issues

$ #13074
PROPOSAL: N/A

Tests

Log in on an account that has a DM chat with many actions (internal engineers, you can use ./script/clitools.sh generator:chatreport for this)
Navigate to a chat that is not the aforementioned chat with many actions
Log out, so that you clear Onyx
Log in.
Make a request money from the person you're chatting with on the DM with many actions
You will get navigated to the report with many actions, make sure that the IOU is shown, and there is a skeleton view above it.

Verify that no errors appear in the JS console

Offline tests

Log in on an account that has a DM chat with many actions (internal engineers, you can use ./script/clitools.sh generator:chatreport for this)
Navigate to a chat that is not the aforementioned chat with many actions
Log out, so that you clear Onyx
Log in.
Set your network connection to offline
Make a request money from the person you're chatting with on the DM with many actions
You will get navigated to the report with many actions, make sure that the IOU is shown, and there is a skeleton view above it. This time the other report actions should not load and you should be shown that you're offline on under the composer.

QA Steps

Do the two tests above
Also navigate between chats and ensure that there are no regressions.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Kapture.2022-12-14.at.10.58.56.mp4

Mobile Web - Chrome

Kapture.2022-12-14.at.11.17.26.mp4

Mobile Web - Safari

Kapture.2022-12-14.at.11.23.22.mp4

Desktop

Kapture.2022-12-14.at.11.26.01.mp4

iOS

Kapture.2022-12-14.at.11.21.48.mp4

Android

Kapture.2022-12-14.at.11.15.09.mp4

melvin-bot · 2022-12-14T19:30:20Z

@parasharrajat @deetergp One of you needs to 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]

deetergp · 2022-12-14T22:48:52Z

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 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

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

deetergp · 2022-12-14T23:27:16Z

Checklist is done and the code looks good to me — leaving it for @parasharrajat to give his seal of approval.

techievivek

While testing this I realized android renders the skeletonUI in opposite direction not sure if this is related to this PR or not. The same thing happens in your and Scott's video as well as for the android mobile.

techievivek · 2022-12-15T06:29:41Z

src/pages/home/report/ReportActionsList.js

+                        }
+
+                        const lastReportAction = _.last(this.props.sortedReportActions);
+                        if (this.props.report.isLoadingReportActions && lastReportAction.sequenceNumber > 0) {


A comment will be helpful regarding why you are doing lastReportAction.sequenceNumber > 0.

…portActionsList

yuwenmemon · 2022-12-15T18:43:19Z

While testing this I realized android renders the skeletonUI in opposite direction not sure if this is related to this PR or not. The same thing happens in your and Scott's video as well as for the android mobile.

Lol wut - hm yeah so I also see this on main/prod when scrolling up so I don't think it's from this PR - I'll make a follow up issue for it!

yuwenmemon · 2022-12-15T18:43:25Z

Updated!

yuwenmemon · 2022-12-15T19:30:53Z

@techievivek created an issue for the right-orientation here: #13636

OSBotify · 2022-12-15T23:30:09Z

✋ 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-15T23:47:45Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	805.280 ms → 813.967 ms (+8.687 ms, +1.1%)
runJsBundle	192.500 ms → 193.875 ms (+1.375 ms, +0.7%)
nativeLaunch	9.129 ms → 9.906 ms (+0.777 ms, +8.5%)
regularAppStart	0.013 ms → 0.014 ms (+0.001 ms, +5.7%)

Show details

Name	Duration
TTI	Baseline Mean: 805.280 ms Stdev: 45.780 ms (5.7%) Runs: 733.7874049991369 745.2957269996405 749.1534189991653 757.3907149992883 761.6277489997447 769.2703699991107 770.1972269993275 771.622827000916 775.7751429993659 776.0893099997193 776.571398999542 778.7221579998732 784.2864469997585 787.5351470001042 794.002915000543 795.0075470004231 799.0544409994036 801.5486999992281 810.5433990005404 811.7991540003568 814.7064660005271 818.9207969997078 830.9786499999464 842.8162799999118 843.1234700009227 846.7306990008801 848.0308090001345 864.2340830005705 873.0365949999541 893.312439000234 938.5090510006994 Current Mean: 813.967 ms Stdev: 35.544 ms (4.4%) Runs: 755.8637329991907 757.6309019997716 768.3134179990739 779.802059000358 782.8118600007147 783.7180240005255 784.7641039993614 785.170012999326 789.3719679992646 790.2667559999973 794.6345499996096 797.573545999825 798.2371660005301 799.1868639998138 806.8310100007802 807.9155080001801 808.377879999578 815.8161149993539 819.5692270006984 821.59451200068 822.7600999996066 828.6122939996421 830.0980389993638 838.9381239991635 844.9051740001887 846.1523030009121 853.6511489991099 863.5237809997052 864.0154599994421 895.207709999755 897.6628530006856
runJsBundle	Baseline Mean: 192.500 ms Stdev: 24.758 ms (12.9%) Runs: 158 160 163 165 166 168 169 174 177 177 178 178 179 180 182 183 188 189 195 196 200 201 202 209 209 213 214 224 234 238 244 247 Current Mean: 193.875 ms Stdev: 20.449 ms (10.5%) Runs: 160 165 168 170 170 174 176 177 178 178 182 183 188 192 192 192 192 193 195 195 198 199 201 205 211 212 213 219 223 225 234 244
nativeLaunch	Baseline Mean: 9.129 ms Stdev: 1.129 ms (12.4%) Runs: 7 8 8 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 11 11 11 12 Current Mean: 9.906 ms Stdev: 1.507 ms (15.2%) Runs: 8 8 8 8 8 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 10 10 10 10 11 11 12 12 12 12 13 14
regularAppStart	Baseline Mean: 0.013 ms Stdev: 0.001 ms (5.0%) Runs: 0.011881999671459198 0.012167001143097878 0.012614000588655472 0.012614000588655472 0.012736000120639801 0.012899000197649002 0.012940000742673874 0.01297999918460846 0.012980001047253609 0.012980999425053596 0.013020999729633331 0.013102000579237938 0.013183999806642532 0.013265000656247139 0.013345999643206596 0.013387000188231468 0.013387000188231468 0.013468999415636063 0.013630999252200127 0.013671999797224998 0.013672001659870148 0.013712000101804733 0.013712000101804733 0.013753000646829605 0.01411999948322773 0.014201000332832336 0.014241000637412071 0.014527000486850739 0.014608001336455345 0.014689000323414803 Current Mean: 0.014 ms Stdev: 0.001 ms (5.0%) Runs: 0.012776000425219536 0.012938998639583588 0.01314299926161766 0.013224000111222267 0.013346999883651733 0.01342800073325634 0.01342800073325634 0.013509001582860947 0.013671999797224998 0.013875000178813934 0.013955999165773392 0.014038000255823135 0.014078998938202858 0.014079000800848007 0.014159999787807465 0.014159999787807465 0.014201000332832336 0.014201000332832336 0.0143630001693964 0.014404000714421272 0.014444999396800995 0.014445001259446144 0.014485999941825867 0.01464799977838993 0.014730000868439674 0.01476999931037426 0.014812000095844269 0.014852000400424004 0.014933999627828598 0.015421999618411064 0.015991000458598137

OSBotify · 2022-12-16T04:00:06Z

🚀 Deployed to staging by @yuwenmemon in version: 1.2.41-0 🚀

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

OSBotify · 2022-12-16T04:31:48Z

🚀 Deployed to staging by @yuwenmemon in version: 1.2.41-0 🚀

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

OSBotify · 2022-12-20T06:05:23Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.41-4 🚀

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

yuwenmemon added 2 commits December 13, 2022 16:39

Make skeleton list footer dynamic based report action list height

41258f4

Make sure the skeleton renders properly and only when it needs to

95f73c0

yuwenmemon self-assigned this Dec 14, 2022

Make sure we don't animate if we're offline

677745a

yuwenmemon marked this pull request as ready for review December 14, 2022 19:29

yuwenmemon requested a review from a team as a code owner December 14, 2022 19:29

melvin-bot bot requested review from deetergp and parasharrajat and removed request for a team December 14, 2022 19:30

deetergp previously approved these changes Dec 14, 2022

View reviewed changes

techievivek reviewed Dec 15, 2022

View reviewed changes

yuwenmemon added 2 commits December 15, 2022 09:26

Merge branch 'main' of github.com:Expensify/App into yuwen-skeletonRe…

b0571a7

…portActionsList

Add comment

2ac451f

yuwenmemon dismissed deetergp’s stale review via 2ac451f December 15, 2022 18:07

deetergp approved these changes Dec 15, 2022

View reviewed changes

yuwenmemon merged commit 3620bf2 into main Dec 15, 2022

yuwenmemon deleted the yuwen-skeletonReportActionsList branch December 15, 2022 23:25

OSBotify mentioned this pull request Dec 16, 2022

Deploy Checklist: New Expensify 2022-12-16 #13654

Closed

25 tasks

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

[HOLD for payment 2022-12-27] White screen appears when loading chat, after being redirected from IOU modal #13074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a skeleton view above optimistic report actions when we're still loading the rest of the report #13590

Show a skeleton view above optimistic report actions when we're still loading the rest of the report #13590

yuwenmemon commented Dec 14, 2022 •

edited

Loading

melvin-bot bot commented Dec 14, 2022

deetergp commented Dec 14, 2022 •

edited

Loading

deetergp commented Dec 14, 2022

techievivek left a comment •

edited

Loading

techievivek Dec 15, 2022

yuwenmemon commented Dec 15, 2022

yuwenmemon commented Dec 15, 2022

yuwenmemon commented Dec 15, 2022

OSBotify commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

OSBotify commented Dec 16, 2022

OSBotify commented Dec 16, 2022

OSBotify commented Dec 20, 2022

Show a skeleton view above optimistic report actions when we're still loading the rest of the report #13590

Show a skeleton view above optimistic report actions when we're still loading the rest of the report #13590

Conversation

yuwenmemon commented Dec 14, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 14, 2022

deetergp commented Dec 14, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

deetergp commented Dec 14, 2022

techievivek left a comment • edited Loading

Choose a reason for hiding this comment

techievivek Dec 15, 2022

Choose a reason for hiding this comment

yuwenmemon commented Dec 15, 2022

yuwenmemon commented Dec 15, 2022

yuwenmemon commented Dec 15, 2022

OSBotify commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Dec 16, 2022

OSBotify commented Dec 16, 2022

OSBotify commented Dec 20, 2022

yuwenmemon commented Dec 14, 2022 •

edited

Loading

deetergp commented Dec 14, 2022 •

edited

Loading

techievivek left a comment •

edited

Loading