Use FullScreenLoadingIndicator in IOU detail modal

[jatinsonijs]

Details

Use common loader component FullScreenLoadingIndicator in IOU Details page

Fixed Issues

$ #14321
PROPOSAL: #14321 (comment)

Tests

1. Open any chat room.
2. Press on any IOU Preview message from chat, if not exist create new one by send/request money.
3. Verify loader is display in center.

• Verify that no errors appear in the JS console

Offline tests

In offline loader will be not display

QA Steps

1. Open any chat room.
2. Press on any IOU Preview message from chat, if not exist create new one by send/request money.
3. Verify loader is display in center.

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • 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 followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• 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
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

IOUDetailsModal

IOUParticipantsPage

Mobile Web - Chrome

IOUDetailsModal

IOUParticipantsPage

Mobile Web - Safari

IOUDetailsModal

IOUParticipantsPage

Desktop

IOUDetailsModal

IOUParticipantsPage

iOS

IOUDetailsModal

IOUParticipantsPage

Android

IOUDetailsModal

IOUParticipantsPage

[melvin-bot]

@PauloGasparSv Please 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]

[Santhosh-Sellavel]

Adding my self as reviewer

[Santhosh-Sellavel]

@jatinsonijs Can you include steps to create IOU if it does not exists already for QA Steps

[mountiny]

@PauloGasparSv taking this one as I am assigned to the issue, I hope thats ok!

[PauloGasparSv]

No problem @mountiny :D

[jatinsonijs]

@jatinsonijs Can you include steps to create IOU if it does not exists already for QA Steps

Done @Santhosh-Sellavel

[Santhosh-Sellavel]

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:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • 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 & Desktop

Screen.Recording.2023-01-19.at.12.57.11.AM.mov

Mobile Web - Chrome & Mobile Web - Safari

Loading_mWeb.mov

iOS & Android

Screen.Recording.2023-01-19.at.12.52.23.AM.mov

[Santhosh-Sellavel]

@mountiny While taping IOU Badge offline it's loading forever

Screen.Recording.2023-01-19.at.1.00.16.AM.mov

[jatinsonijs]

While taping IOU Badge offline it's loading forever

@Santhosh-Sellavel is it happening every time ?, actually I have faced same issue yesterday but it Is not reproducing again.

[Santhosh-Sellavel]

Yes

While taping IOU Badge offline it's loading forever

@Santhosh-Sellavel is it happening every time ?, actually I have faced same issue yesterday but it Is not reproducing again.

Yes, happening everytime!

[jatinsonijs]

I have tried to reproduce it but loader not display

loader.mp4

[Santhosh-Sellavel]

@jatinsonijs

Screen.Recording.2023-01-19.at.1.22.32.AM.mov

[mountiny]

@Santhosh-Sellavel Can you check if you have the IOU report details in your indexedDB locally?

I assume however, this is happening in main as well, this PR is not changing this behaviour

[Santhosh-Sellavel]

@jatinsonijs Seems it happens if some data is not fetched completely also I think this is a bit out of scope.

@mountiny Let me know what should we do here, thanks!

[Santhosh-Sellavel]

LGTM, tests well!

[Santhosh-Sellavel]

@mountiny Can we delete this comment, I think the check is failing because of that!

[mountiny]

Done!

[jatinsonijs]

Yes @mountiny, @Santhosh-Sellavel its happening due to data not fetched completely, I have found way to reproduce it

Open IOU Details page
wait for data loading
Go offline -> Switch to slow 3G in chrome tool.
Wait for loader to be display.
Once loader displayed immediately go offline.
In this case loader will stuck

loader-stuck.mp4

Its not related to this PR, if you think it should be fixed I can report it on slack channel

[mountiny]

@jatinsonijs @Santhosh-Sellavel Should we also do this for the IOUParticipantsPage as was mentioned in the issue? Any reason why not?

[jatinsonijs]

Yes @mountiny we can do it, but there is not any clear steps for QA to test it. We can test it manually by change condition in code.

[mountiny]

@jatinsonijs Ah you mean that the loading page there is not triggered in normal flow? I would still rather update the styles same as we do in IOUDetails page and just dont include it in the QA, since the styles / views are same we can expect it will work fine.

[Santhosh-Sellavel]

@jatinsonijs

App/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsPage.js

Lines 54 to 56 in 5370c36

	<View style={styles.pageWrapper}>
	<ActivityIndicator color={themeColors.text} />
	</View>

Replacing the above with the below snippet should do

<View style={styles.flex1}>
      <FullScreenLoadingIndicator />
  </View>

cc: @mountiny

[mountiny]

That seems reasonable, I mean if it does not really show for user, we might want to refactor this more but thats for later.

[Santhosh-Sellavel]

@jatinsonijs please add screenshots once the changes are done, thanks!

@mountiny Once changes are done, feel free to merge as it would not break anything, I tested it works for all platforms!

[jatinsonijs]

Changes are done @Santhosh-Sellavel and screenshots added.

cc @mountiny

[mountiny]

Thank you @jatinsonijs @Santhosh-Sellavel !

[OSBotify]

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

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
App start TTI	672.039 ms → 694.734 ms (+22.696 ms, +3.4%)
App start runJsBundle	183.290 ms → 194.000 ms (+10.710 ms, +5.8%)
Open Search Page TTI	595.002 ms → 600.079 ms (+5.077 ms, +0.9%)
App start regularAppStart	0.016 ms → 0.014 ms (-0.002 ms, -10.4%)
App start nativeLaunch	19.586 ms → 18.935 ms (-0.651 ms, -3.3%)

Show details

Name	Duration
App start TTI	Baseline Mean: 672.039 ms Stdev: 24.132 ms (3.6%) Runs: 627.6952010020614 632.1126350015402 638.3559530004859 639.4095470011234 644.7382839992642 645.2582169994712 652.7438059970737 652.9927700012922 656.3108569979668 659.3305459991097 660.6222229972482 663.7414819970727 663.9122600033879 667.1332739964128 667.5651540011168 667.7722650021315 671.2471529990435 672.2051149979234 674.4183520004153 680.9892230033875 685.1110019981861 685.5854419991374 685.6148620024323 686.7481319978833 689.7484370023012 689.9635440036654 691.4977810010314 693.6834470033646 699.5535300001502 714.6573460027575 721.506429001689 723.0100449994206 Current Mean: 694.734 ms Stdev: 25.668 ms (3.7%) Runs: 654.0904569998384 655.3563719987869 663.0420389994979 664.5850950032473 668.3320119976997 671.1808919981122 673.9576829969883 674.3920129984617 675.8193520009518 678.0756329968572 679.3654529973865 685.2167150005698 685.2948179990053 686.447131998837 688.9861200004816 691.8295139968395 693.0125460028648 695.7848099991679 702.7760649994016 704.5601250007749 709.1277280002832 709.2925959974527 709.3193430006504 710.3268909975886 722.59220200032 726.6419759988785 733.1609669998288 738.3185949996114 743.1720009967685 747.9719749987125
App start runJsBundle	Baseline Mean: 183.290 ms Stdev: 14.807 ms (8.1%) Runs: 158 162 164 166 166 168 170 171 172 175 175 175 177 177 178 183 183 187 187 189 191 193 194 194 196 199 199 202 206 207 218 Current Mean: 194.000 ms Stdev: 17.876 ms (9.2%) Runs: 166 167 168 168 169 174 175 177 181 184 185 185 190 192 192 195 195 195 197 198 199 200 205 206 211 213 215 216 217 222 224 227
Open Search Page TTI	Baseline Mean: 595.002 ms Stdev: 16.064 ms (2.7%) Runs: 563.3994150012732 570.5117999985814 572.6477869972587 572.7320969998837 573.0794280022383 578.9152429997921 581.640381000936 584.9482830017805 586.203938998282 586.3714599981904 586.3867599964142 587.2460540011525 589.4464519992471 590.8527429997921 591.9772949963808 593.5169270038605 594.2745769992471 597.9774580001831 600.3520100042224 600.5194090008736 602.8408610001206 603.6414390057325 604.0813390016556 604.9992269948125 606.956705994904 607.0384530052543 609.7588299959898 612.3106689974666 613.9446210041642 616.875813998282 617.7643240019679 636.8536380007863 Current Mean: 600.079 ms Stdev: 16.375 ms (2.7%) Runs: 577.2987059950829 577.7219650000334 577.9698079973459 579.2768559977412 579.9892170056701 582.8848879933357 583.8459070026875 586.056558996439 587.6841639950871 589.2643240019679 590.7786459997296 595.8977869972587 597.113240994513 597.9556079953909 598.531860999763 598.6390379965305 598.7761240005493 599.6706549972296 599.9027099981904 601.1893309950829 602.6315519958735 602.7387700006366 605.1318770051003 608.0211590006948 609.9301759973168 610.1114099994302 611.9945880025625 623.6447350010276 628.6658120006323 631.7631840035319 632.615845002234 634.843221001327
App start regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.001 ms (8.0%) Runs: 0.013876006007194519 0.014607995748519897 0.014688998460769653 0.014729999005794525 0.01477000117301941 0.015096001327037811 0.015096001327037811 0.015178002417087555 0.015218004584312439 0.015258997678756714 0.015300005674362183 0.015341006219387054 0.015422001481056213 0.015583999454975128 0.015583999454975128 0.015706002712249756 0.015991002321243286 0.015991002321243286 0.016112998127937317 0.016276001930236816 0.01635800302028656 0.016479000449180603 0.016520000994205475 0.016520000994205475 0.01721099764108658 0.017212003469467163 0.019368000328540802 0.019774995744228363 Current Mean: 0.014 ms Stdev: 0.001 ms (3.9%) Runs: 0.01326499879360199 0.013305999338626862 0.013631001114845276 0.01367199420928955 0.013712994754314423 0.013794004917144775 0.013875000178813934 0.013957001268863678 0.013957001268863678 0.014038003981113434 0.014078997075557709 0.014079004526138306 0.014118999242782593 0.014118999242782593 0.014241993427276611 0.014242000877857208 0.014322996139526367 0.014364004135131836 0.014403998851776123 0.014403998851776123 0.014405004680156708 0.01452600210905075 0.014567002654075623 0.014647997915744781 0.014769993722438812 0.015177994966506958 0.015462003648281097 0.015666000545024872
App start nativeLaunch	Baseline Mean: 19.586 ms Stdev: 1.609 ms (8.2%) Runs: 17 17 17 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 21 21 22 22 22 23 Current Mean: 18.935 ms Stdev: 1.480 ms (7.8%) Runs: 17 17 17 17 18 18 18 18 18 18 18 18 18 18 18 18 18 19 19 19 20 20 20 20 20 21 21 21 21 22 22

[OSBotify]

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.57-0 🚀

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

[OSBotify]

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.57-3 🚀

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