Skip to content
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

Add simple loader to payment method list #11592

Merged
merged 32 commits into from
Jan 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 5 additions & 14 deletions src/components/CurrentWalletBalance.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
import React from 'react';
import {ActivityIndicator} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
import {withOnyx} from 'react-native-onyx';
import styles from '../styles/styles';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import compose from '../libs/compose';
import themeColors from '../styles/themes/default';
import ONYXKEYS from '../ONYXKEYS';
import Text from './Text';

Expand All @@ -25,21 +22,15 @@ const propTypes = {
};

const defaultProps = {
userWallet: {},
userWallet: {

// Default to zero if userWallet and currentBalance is not set yet to avoid NaN
currentBalance: 0,
},
balanceStyles: [],
};

const CurrentWalletBalance = (props) => {
if (_.isEmpty(props.userWallet)) {
return (
<ActivityIndicator
color={themeColors.text}
size="large"
style={styles.pv5}
/>
);
}

const formattedBalance = props.numberFormat(
props.userWallet.currentBalance / 100, // Divide by 100 because balance is in cents
{style: 'currency', currency: 'USD'},
Expand Down
9 changes: 8 additions & 1 deletion src/pages/settings/Payments/PaymentMethodList.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ const propTypes = {
/** Whether the add Payment button be shown on the list */
shouldShowAddPaymentMethodButton: PropTypes.bool,

/** Are we loading payment methods? */
isLoadingPaymentMethods: PropTypes.bool,

/** Type to filter the payment Method list */
filterType: PropTypes.oneOf([CONST.PAYMENT_METHODS.DEBIT_CARD, CONST.PAYMENT_METHODS.BANK_ACCOUNT, '']),

Expand Down Expand Up @@ -74,6 +77,7 @@ const defaultProps = {
walletLinkedAccountID: 0,
walletLinkedAccountType: '',
},
isLoadingPaymentMethods: true,
shouldShowAddPaymentMethodButton: true,
filterType: '',
actionPaymentMethodType: '',
Expand Down Expand Up @@ -231,7 +235,7 @@ class PaymentMethodList extends Component {
text={this.props.translate('paymentMethodList.addPaymentMethod')}
icon={Expensicons.CreditCard}
onPress={e => this.props.onPress(e)}
isDisabled={this.props.isLoadingPayments || isOffline}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB - was isLoadingPayments ever passed by anything? I don't see it anywhere else in the code...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it was.

isDisabled={this.props.isLoadingPaymentMethods || isOffline}
style={[styles.mh4, styles.buttonCTA]}
iconStyles={[styles.buttonCTAIcon]}
key="addPaymentMethodButton"
Expand Down Expand Up @@ -261,6 +265,9 @@ export default compose(
cardList: {
key: ONYXKEYS.CARD_LIST,
},
isLoadingPaymentMethods: {
key: ONYXKEYS.IS_LOADING_PAYMENT_METHODS,
},
payPalMeData: {
key: ONYXKEYS.PAYPAL,
},
Expand Down
46 changes: 37 additions & 9 deletions src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import {
View, InteractionManager, LayoutAnimation,
ActivityIndicator, View, InteractionManager, LayoutAnimation,
} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
Expand Down Expand Up @@ -33,6 +33,7 @@ import * as PaymentUtils from '../../../../libs/PaymentUtils';
import OfflineWithFeedback from '../../../../components/OfflineWithFeedback';
import ConfirmContent from '../../../../components/ConfirmContent';
import Button from '../../../../components/Button';
import themeColors from '../../../../styles/themes/default';

class BasePaymentsPage extends React.Component {
constructor(props) {
Expand All @@ -42,6 +43,7 @@ class BasePaymentsPage extends React.Component {
shouldShowAddPaymentMenu: false,
shouldShowDefaultDeleteMenu: false,
shouldShowPasswordPrompt: false,
shouldShowLoadingSpinner: false,
isSelectedPaymentMethodDefault: false,
selectedPaymentMethod: {},
formattedSelectedPaymentMethod: {
Expand All @@ -65,6 +67,8 @@ class BasePaymentsPage extends React.Component {
this.navigateToTransferBalancePage = this.navigateToTransferBalancePage.bind(this);
this.setMenuPosition = this.setMenuPosition.bind(this);
this.listHeaderComponent = this.listHeaderComponent.bind(this);

this.debounceSetShouldShowLoadingSpinner = _.debounce(this.setShouldShowLoadingSpinner.bind(this), CONST.TIMING.SHOW_LOADING_SPINNER_DEBOUNCE_TIME);
}

componentDidMount() {
Expand All @@ -76,12 +80,29 @@ class BasePaymentsPage extends React.Component {
this.setMenuPosition();
}

// If the user was previously offline, skip debouncing showing the loader
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (prevProps.network.isOffline && !this.props.network.isOffline) {
this.setShouldShowLoadingSpinner();
} else {
this.debounceSetShouldShowLoadingSpinner();
}

// previously online OR currently offline, skip fetch
if (!prevProps.network.isOffline || this.props.network.isOffline) {
return;
}

this.fetchData();
}

setShouldShowLoadingSpinner() {
Copy link
Contributor

@neil-marcellini neil-marcellini Jan 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// In order to prevent a loop, only update state of the spinner if there is a change
const shouldShowLoadingSpinner = this.props.isLoadingPaymentMethods || false;
if (shouldShowLoadingSpinner !== this.state.shouldShowLoadingSpinner) {
this.setState({shouldShowLoadingSpinner: this.props.isLoadingPaymentMethods && !this.props.network.isOffline});
}
}

setMenuPosition() {
if (!this.state.addPaymentMethodButton) {
return;
Expand Down Expand Up @@ -268,14 +289,18 @@ class BasePaymentsPage extends React.Component {
{Permissions.canUseWallet(this.props.betas) && (
<>
<View style={[styles.mv5]}>
<OfflineWithFeedback
pendingAction={CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}
errors={this.props.walletTerms.errors}
onClose={PaymentMethods.clearWalletTermsError}
errorRowStyles={[styles.ml10, styles.mr2]}
>
<CurrentWalletBalance />
</OfflineWithFeedback>
{this.state.shouldShowLoadingSpinner ? (
<ActivityIndicator color={themeColors.spinner} size="large" />
) : (
<OfflineWithFeedback
pendingAction={CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}
errors={this.props.walletTerms.errors}
onClose={PaymentMethods.clearWalletTermsError}
errorRowStyles={[styles.ml10, styles.mr2]}
>
<CurrentWalletBalance />
</OfflineWithFeedback>
)}
</View>
{this.props.userWallet.currentBalance > 0 && (
<KYCWall
Expand Down Expand Up @@ -497,5 +522,8 @@ export default compose(
walletTerms: {
key: ONYXKEYS.WALLET_TERMS,
},
isLoadingPaymentMethods: {
key: ONYXKEYS.IS_LOADING_PAYMENT_METHODS,
},
}),
)(BasePaymentsPage);