-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix: Add payment dynamic #7566
Merged
Merged
Fix: Add payment dynamic #7566
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
056ae84
Fix add payment position dynamic
phivh bf98fb6
Merge branch 'main' into fix/add-payment-dynamic
phivh 789c3ec
cleanup
phivh 9de73c1
cleanup
phivh 2298ebc
Change usage of boolean prop name
phivh 62f1a74
Merge branch 'main' into fix/add-payment-dynamic
phivh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import React from 'react'; | ||
import {propTypes, defaultProps} from './kycWallPropTypes'; | ||
import BaseKYCWall from './BaseKYCWall'; | ||
|
||
const KYCWall = props => ( | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
<BaseKYCWall {...props} shouldListenForResize /> | ||
); | ||
|
||
KYCWall.propTypes = propTypes; | ||
KYCWall.defaultProps = defaultProps; | ||
KYCWall.displayName = 'KYCWall'; | ||
|
||
export default KYCWall; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New line above |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import BaseKYCWall from './BaseKYCWall'; | ||
|
||
export default BaseKYCWall; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import PropTypes from 'prop-types'; | ||
import userWalletPropTypes from '../../pages/EnablePayments/userWalletPropTypes'; | ||
|
||
const propTypes = { | ||
/** Route for the Add Bank Account screen for a given navigation stack */ | ||
addBankAccountRoute: PropTypes.string.isRequired, | ||
|
||
/** Route for the Add Debit Card screen for a given navigation stack */ | ||
addDebitCardRoute: PropTypes.string.isRequired, | ||
|
||
/** Route for the KYC enable payments screen for a given navigation stack */ | ||
enablePaymentsRoute: PropTypes.string.isRequired, | ||
|
||
/** Where to place the popover */ | ||
popoverPlacement: PropTypes.string, | ||
|
||
/** Listen for window resize event on web and desktop */ | ||
shouldListenForResize: PropTypes.bool, | ||
|
||
...userWalletPropTypes, | ||
}; | ||
|
||
const defaultProps = { | ||
// eslint-disable-next-line react/default-props-match-prop-types | ||
userWallet: {}, | ||
popoverPlacement: 'top', | ||
shouldListenForResize: false, | ||
}; | ||
|
||
export {propTypes, defaultProps}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,59 +1,35 @@ | ||
import React from 'react'; | ||
import {View, TouchableOpacity} from 'react-native'; | ||
import {withOnyx} from 'react-native-onyx'; | ||
import PropTypes from 'prop-types'; | ||
import PaymentMethodList from './PaymentMethodList'; | ||
import ROUTES from '../../../ROUTES'; | ||
import HeaderWithCloseButton from '../../../components/HeaderWithCloseButton'; | ||
import PasswordPopover from '../../../components/PasswordPopover'; | ||
import ScreenWrapper from '../../../components/ScreenWrapper'; | ||
import Navigation from '../../../libs/Navigation/Navigation'; | ||
import styles from '../../../styles/styles'; | ||
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize'; | ||
import compose from '../../../libs/compose'; | ||
import KeyboardAvoidingView from '../../../components/KeyboardAvoidingView/index'; | ||
import * as BankAccounts from '../../../libs/actions/BankAccounts'; | ||
import Popover from '../../../components/Popover'; | ||
import MenuItem from '../../../components/MenuItem'; | ||
import Text from '../../../components/Text'; | ||
import * as PaymentMethods from '../../../libs/actions/PaymentMethods'; | ||
import getClickedElementLocation from '../../../libs/getClickedElementLocation'; | ||
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions'; | ||
import CurrentWalletBalance from '../../../components/CurrentWalletBalance'; | ||
import ONYXKEYS from '../../../ONYXKEYS'; | ||
import Permissions from '../../../libs/Permissions'; | ||
import ConfirmPopover from '../../../components/ConfirmPopover'; | ||
import AddPaymentMethodMenu from '../../../components/AddPaymentMethodMenu'; | ||
import CONST from '../../../CONST'; | ||
import * as Expensicons from '../../../components/Icon/Expensicons'; | ||
import walletTransferPropTypes from './walletTransferPropTypes'; | ||
import ConfirmModal from '../../../components/ConfirmModal'; | ||
import KYCWall from '../../../components/KYCWall'; | ||
import PaymentMethodList from '../PaymentMethodList'; | ||
import ROUTES from '../../../../ROUTES'; | ||
import HeaderWithCloseButton from '../../../../components/HeaderWithCloseButton'; | ||
import PasswordPopover from '../../../../components/PasswordPopover'; | ||
import ScreenWrapper from '../../../../components/ScreenWrapper'; | ||
import Navigation from '../../../../libs/Navigation/Navigation'; | ||
import styles from '../../../../styles/styles'; | ||
import withLocalize from '../../../../components/withLocalize'; | ||
import compose from '../../../../libs/compose'; | ||
import KeyboardAvoidingView from '../../../../components/KeyboardAvoidingView/index'; | ||
import * as BankAccounts from '../../../../libs/actions/BankAccounts'; | ||
import Popover from '../../../../components/Popover'; | ||
import MenuItem from '../../../../components/MenuItem'; | ||
import Text from '../../../../components/Text'; | ||
import * as PaymentMethods from '../../../../libs/actions/PaymentMethods'; | ||
import getClickedElementLocation from '../../../../libs/getClickedElementLocation'; | ||
import withWindowDimensions from '../../../../components/withWindowDimensions'; | ||
import CurrentWalletBalance from '../../../../components/CurrentWalletBalance'; | ||
import ONYXKEYS from '../../../../ONYXKEYS'; | ||
import Permissions from '../../../../libs/Permissions'; | ||
import ConfirmPopover from '../../../../components/ConfirmPopover'; | ||
import AddPaymentMethodMenu from '../../../../components/AddPaymentMethodMenu'; | ||
import CONST from '../../../../CONST'; | ||
import * as Expensicons from '../../../../components/Icon/Expensicons'; | ||
import ConfirmModal from '../../../../components/ConfirmModal'; | ||
import KYCWall from '../../../../components/KYCWall'; | ||
import {propTypes, defaultProps} from './paymentsPagePropTypes'; | ||
|
||
const propTypes = { | ||
/** Wallet balance transfer props */ | ||
walletTransfer: walletTransferPropTypes, | ||
|
||
/** List of betas available to current user */ | ||
betas: PropTypes.arrayOf(PropTypes.string), | ||
|
||
/** Are we loading payment methods? */ | ||
isLoadingPaymentMethods: PropTypes.bool, | ||
|
||
...withLocalizePropTypes, | ||
|
||
...windowDimensionsPropTypes, | ||
}; | ||
|
||
const defaultProps = { | ||
walletTransfer: { | ||
shouldShowConfirmModal: false, | ||
}, | ||
betas: [], | ||
isLoadingPaymentMethods: true, | ||
}; | ||
|
||
class PaymentsPage extends React.Component { | ||
class BasePaymentsPage extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
|
||
|
@@ -94,6 +70,20 @@ class PaymentsPage extends React.Component { | |
} | ||
} | ||
|
||
/** | ||
* Set position of the payment menu | ||
* | ||
* @param {Object} position | ||
*/ | ||
setPositionAddPaymentMenu(position) { | ||
this.setState({ | ||
anchorPositionTop: position.bottom, | ||
|
||
// We want the position to be 13px to the right of the left border | ||
anchorPositionLeft: position.left + 13, | ||
}); | ||
} | ||
|
||
/** | ||
* Display the delete/default menu, or the add payment method menu | ||
* | ||
|
@@ -102,7 +92,13 @@ class PaymentsPage extends React.Component { | |
* @param {String} account | ||
*/ | ||
paymentMethodPressed(nativeEvent, accountType, account) { | ||
const position = getClickedElementLocation(nativeEvent); | ||
let position = getClickedElementLocation(nativeEvent); | ||
if (this.props.shouldListenForResize) { | ||
window.addEventListener('resize', () => { | ||
position = getClickedElementLocation(nativeEvent); | ||
this.setPositionAddPaymentMenu(position); | ||
}); | ||
} | ||
if (accountType) { | ||
let formattedSelectedPaymentMethod; | ||
if (accountType === CONST.PAYMENT_METHODS.PAYPAL) { | ||
|
@@ -131,21 +127,15 @@ class PaymentsPage extends React.Component { | |
shouldShowDefaultDeleteMenu: true, | ||
selectedPaymentMethod: account, | ||
selectedPaymentMethodType: accountType, | ||
anchorPositionTop: position.bottom, | ||
|
||
// We want the position to be 13px to the right of the left border | ||
anchorPositionLeft: position.left + 13, | ||
formattedSelectedPaymentMethod, | ||
}); | ||
} else { | ||
this.setState({ | ||
shouldShowAddPaymentMenu: true, | ||
anchorPositionTop: position.bottom, | ||
|
||
// We want the position to be 13px to the right of the left border | ||
anchorPositionLeft: position.left + 13, | ||
}); | ||
this.setPositionAddPaymentMenu(position); | ||
return; | ||
} | ||
this.setState({ | ||
shouldShowAddPaymentMenu: true, | ||
}); | ||
this.setPositionAddPaymentMenu(position); | ||
} | ||
|
||
/** | ||
|
@@ -178,13 +168,19 @@ class PaymentsPage extends React.Component { | |
* Hide the add payment modal | ||
*/ | ||
hideAddPaymentMenu() { | ||
if (this.props.shouldListenForResize) { | ||
window.removeEventListener('resize', null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove eventListener in |
||
} | ||
this.setState({shouldShowAddPaymentMenu: false}); | ||
} | ||
|
||
/** | ||
* Hide the default / delete modal | ||
*/ | ||
hideDefaultDeleteMenu() { | ||
if (this.props.shouldListenForResize) { | ||
window.removeEventListener('resize', null); | ||
} | ||
this.setState({shouldShowDefaultDeleteMenu: false}); | ||
} | ||
|
||
|
@@ -393,8 +389,8 @@ class PaymentsPage extends React.Component { | |
} | ||
} | ||
|
||
PaymentsPage.propTypes = propTypes; | ||
PaymentsPage.defaultProps = defaultProps; | ||
BasePaymentsPage.propTypes = propTypes; | ||
BasePaymentsPage.defaultProps = defaultProps; | ||
|
||
export default compose( | ||
withWindowDimensions, | ||
|
@@ -411,4 +407,4 @@ export default compose( | |
initWithStoredValues: false, | ||
}, | ||
}), | ||
)(PaymentsPage); | ||
)(BasePaymentsPage); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import React from 'react'; | ||
import BasePaymentsPage from './BasePaymentsPage'; | ||
|
||
const PaymentsPage = () => ( | ||
<BasePaymentsPage shouldListenForResize /> | ||
); | ||
|
||
PaymentsPage.displayName = 'PaymentsPage'; | ||
|
||
export default PaymentsPage; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import BasePaymentsPage from './BasePaymentsPage'; | ||
|
||
export default BasePaymentsPage; |
33 changes: 33 additions & 0 deletions
33
src/pages/settings/Payments/PaymentsPage/paymentsPagePropTypes.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import PropTypes from 'prop-types'; | ||
import walletTransferPropTypes from '../walletTransferPropTypes'; | ||
import {withLocalizePropTypes} from '../../../../components/withLocalize'; | ||
import {windowDimensionsPropTypes} from '../../../../components/withWindowDimensions'; | ||
|
||
const propTypes = { | ||
/** Wallet balance transfer props */ | ||
walletTransfer: walletTransferPropTypes, | ||
|
||
/** List of betas available to current user */ | ||
betas: PropTypes.arrayOf(PropTypes.string), | ||
|
||
/** Are we loading payment methods? */ | ||
isLoadingPaymentMethods: PropTypes.bool, | ||
|
||
/** Listen for window resize event on web and desktop. */ | ||
shouldListenForResize: PropTypes.bool, | ||
|
||
...withLocalizePropTypes, | ||
|
||
...windowDimensionsPropTypes, | ||
}; | ||
|
||
const defaultProps = { | ||
walletTransfer: { | ||
shouldShowConfirmModal: false, | ||
}, | ||
betas: [], | ||
isLoadingPaymentMethods: true, | ||
shouldListenForResize: false, | ||
}; | ||
|
||
export {propTypes, defaultProps}; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hmm, this is gonna bind a new event listener each time. That can have unintended consequences. The best practice would be to set an event listener once on component init and then clean it up on unmount...
Here's an example of what that looks like using a class component
https://reactjs.org/docs/hooks-effect.html#example-using-classes-1
cc @rushatgabhane @deetergp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we already added a remove listener when the modal close. The scenario is add listener when the modal appears and remove listener when the modal closes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix this. Here are the instructions...
componentDidMount()
componentWillUnmount()
Generally speaking even if we are focusing on the web side for consistency we should use the
Dimensions
utility. React native web is able to use this.https://reactnative.dev/docs/0.66/dimensions#addeventlistener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phivh right now, we're adding a event listener each time the window is resized.
So yeah, let's create a follow-up PR to fix this. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let me take into this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcaaron @deetergp @rushatgabhane created follow-up PR for this one. Please take a look when you guys have a chance.
#7847