Add simple loader to payment method list #11592
Conversation
There was a problem hiding this comment.
So this is working for me and code looks good. Two questions though:
- It's failing the security/snyk check. I imagine that's nothing to do with your code? Maybe merge main again - though if it JUST happened then we might have to wait.
- This is more a design thought so I guess technically NAB but it bothers me that the loader has less height than a single bank account so it jumps when the list eventually loads (if you only have one account). Is there a way to add padding to it?
|
Can you find a way to show the loader right away? |
|
I'm sure this isn't a priority right now but figured I'd check in to see how things are going - let me know if there's something new to review and happy to do so! |
|
We've since moved the payment method button to the bottom, now I'm struggling to keep it at the bottom |
|
So it should always show the spinner first now. Updated and ready for review |
|
Conflicts |
melvin-bot
bot
requested review from
neil-marcellini and
rushatgabhane
and removed request for
a team
|
@rushatgabhane @neil-marcellini 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] |
|
👀 |
There was a problem hiding this comment.
The code looks pretty good to me, but I do have some questions. I also noticed that the add payment method button is enabled before the payment methods have loaded.
When the user is offline and we haven't fetched the payment methods, why do we show that they have none? That seems wrong.
web.mov
I think thats a good suggestion, but this PR has dragged its feet long enough. I'd rather just get this out and then we can make additional changes after |
👍 I agree
Yes I agree that's confusing but it's also a very rare flow. How often will someone sign in, add a payment method, sign out, sign back in, go offline, and then open this page? Although it's rare, I tend to think that these payment pages should use an offline blocking pattern, or at least give some indication that the data could be stale, because if you are offline and you suddenly see $0 in your wallet when it was $1,000 previously you might be alarmed. In another PR I implemented a partial offline blocking pattern for the Reimburse expenses page due to similar reasoning. |
There was a problem hiding this comment.
Looking good, very close. The tests work well but I did notice a flashing of the loading indicator which would be a regression IMO. I think we should debounce showing the loading indicator in a similar manner to what I did here.
Screen.Recording.2023-01-03.at.11.31.00.AM.mov
There was a problem hiding this comment.
Unfortunately I found a few problems.
- If you are offline and go to the payments page there is a brief loader. I don't think there should be a loader at all in this case.
- The same is true when you are online and the data is fetched very quickly. I think it might be better to render nothing or the last balance?
- If you go to the payments page twice while offline there is an infinite spinner
- componentDidUpdate is getting called in a loop
web.mov
infinite-loader.mov
| this.fetchData(); | ||
| } | ||
|
|
||
| setShouldShowLoadingSpinner() { |
There was a problem hiding this comment.
This function is being called in a loop. Every time we fetchData we update the component, then we call this function, then we fetch data again.
Edit: Hmm my previous explanation was wrong, but if I put a console.log here I see it getting called repeatedly.
| @@ -76,12 +80,25 @@ class BasePaymentsPage extends React.Component { | |||
| this.setMenuPosition(); | |||
| } | |||
|
|
|||
| // If the user was previously offline, skip debouncing showing the loader | |||
There was a problem hiding this comment.
Do we need to skip debouncing? If we come back online we should fetch data again and show a loader if it takes a while.
There was a problem hiding this comment.
The issue is that we also grey out the wallet amount when offline, so it does a strange thing where, the number will go from grey to white, then the loader will take over
There was a problem hiding this comment.
Ok that makes sense. I'm a bit confused still because if you are on the page and go online->offline->online (slow 3g) then you don't see the loader right away. I would say this is NAB at this point but would be nice to fix.
Screen.Recording.2023-01-19.at.10.41.53.AM.mov
There was a problem hiding this comment.
As requested, I will review only the QA steps and videos first. I think it looks good!
Maybe we could also test going to the page while online, going offline, and going back online to make sure that changing offline state works while the component is mounted. Also please clarify / cleanup tests/QA in the description. I think the steps would be the same for both right? Next I'll test myself and look at the code 🙂
| @@ -76,12 +80,25 @@ class BasePaymentsPage extends React.Component { | |||
| this.setMenuPosition(); | |||
| } | |||
|
|
|||
| // If the user was previously offline, skip debouncing showing the loader | |||
There was a problem hiding this comment.
Ok that makes sense. I'm a bit confused still because if you are on the page and go online->offline->online (slow 3g) then you don't see the loader right away. I would say this is NAB at this point but would be nice to fix.
Screen.Recording.2023-01-19.at.10.41.53.AM.mov
|
Updated! |
|
@rushatgabhane could you please review today? |
There was a problem hiding this comment.
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 Issuessection 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
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - 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 verified the steps for local testing are in the
- 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.
toggleReportand notonIconClick). - 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 Copylabel 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
- 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.
- 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 usingAvatarhave 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
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - 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
Avataris modified, I verified thatAvataris 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
ScrollViewcomponent 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
Screen.Recording.2023-01-20.at.12.53.59.AM.mov
Mobile Web - Chrome
WhatsApp.Video.2023-01-20.at.01.00.37.mp4
Mobile Web - Safari
Screen.Recording.2023-01-20.at.2.48.31.AM.mov
Desktop
Screen.Recording.2023-01-20.at.2.50.43.AM.mov
iOS
Screen.Recording.2023-01-20.at.2.44.43.AM.mov
Android
screen-20230120-005740.mp4
|
Edit: Chatted 1:1 with @ctkochan22 This is not an issue. I was simulating failed network requests by disabling internet access. This is different as the sim doesn't think it's offline. Retested by using the force offline toggle in Preferences, and I don't see an infinite loader anymore. I see a infinite loader on iOS and iOS Safari. Screen.Recording.2023-01-20.at.1.17.39.AM.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
|
🚀 Deployed to staging by https://github.com/ctkochan22 in version: 1.2.57-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.57-3 🚀
|
|
Not showing up on #11460 |
@Expensify/pullerbear @nkuoch
Details
Adds a simple loader
Fixed Issues
$ #11460
$ #11460
Tests/QA
PR Review Checklist
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Test
Test
Video Example: https://recordit.co/GoFOIWOdA8
Test
Video Example: https://recordit.co/0SEts8J1ak
Screenshots
Web
Mobile Web - Chrome
https://recordit.co/EJwbMELIz5
Mobile Web - Safari
https://recordit.co/yOIbhuFCWk
Desktop
https://recordit.co/vCrCHuYg3d
iOS
https://recordit.co/cuRlGUY8jG