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.

Sign up for GitHub

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 popover position for payment buttons #28744

Merged
merged 5 commits into from
Oct 5, 2023
+46 −9
Merged

Fix popover position for payment buttons #28744

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ccd1b31
Fix popover position for payment buttons
Nikhil-Vats Oct 3, 2023
e5652b6
Merge branch 'main' into 26070_fix_popover_position
Nikhil-Vats Oct 5, 2023
46c3045
Add safety check for target element
Nikhil-Vats Oct 5, 2023
5b7f439
Improve comment
Nikhil-Vats Oct 5, 2023
3ba03d1
Add empty line before comment
Nikhil-Vats Oct 5, 2023
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
12 changes: 12 additions & 0 deletions src/components/AddPaymentMethodMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import _ from 'underscore';
import React from 'react';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
Expand All @@ -24,6 +25,12 @@ const propTypes = {
vertical: PropTypes.number,
}),

/** Where the popover should be positioned relative to the anchor points. */
anchorAlignment: PropTypes.shape({
horizontal: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL)),
vertical: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_VERTICAL)),
}),

/** List of betas available to current user */
betas: PropTypes.arrayOf(PropTypes.string),

Expand All @@ -35,6 +42,10 @@ const propTypes = {

const defaultProps = {
anchorPosition: {},
anchorAlignment: {
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
},
betas: [],
anchorRef: () => {},
};
Expand All @@ -45,6 +56,7 @@ function AddPaymentMethodMenu(props) {
isVisible={props.isVisible}
onClose={props.onClose}
anchorPosition={props.anchorPosition}
anchorAlignment={props.anchorAlignment}
anchorRef={props.anchorRef}
onItemSelected={props.onClose}
menuItems={[
Expand Down
1 change: 1 addition & 0 deletions src/components/ButtonWithDropdownMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ function ButtonWithDropdownMenu(props) {
) : (
<Button
success
ref={props.buttonRef}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aimane-chnaif I added this to fix this bug because I also found it during testing.

pressOnEnter={props.pressOnEnter}
isDisabled={props.isDisabled}
style={[styles.w100, ...props.style]}
Expand Down
13 changes: 7 additions & 6 deletions src/components/KYCWall/BaseKYCWall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ class KYCWall extends React.Component {
* @returns {Object}
*/
getAnchorPosition(domRect) {
if (this.props.popoverPlacement === 'bottom') {
if (this.props.anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

explanation here -

App/src/components/ButtonWithDropdownMenu.js

Lines 91 to 94 in f77a5a5

props.anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP
? y + h + CONST.MODAL.POPOVER_MENU_PADDING // if vertical anchorAlignment is TOP, menu will open below the button and we need to add the height of button and padding
: y - CONST.MODAL.POPOVER_MENU_PADDING, // if it is BOTTOM, menu will open above the button so NO need to add height but DO subtract padding
});

return {
anchorPositionVertical: domRect.top + (domRect.height - 2),
anchorPositionVertical: domRect.top + domRect.height + CONST.MODAL.POPOVER_MENU_PADDING,
anchorPositionHorizontal: domRect.left + 20,
};
}
Expand Down Expand Up @@ -92,15 +92,15 @@ class KYCWall extends React.Component {
* If they do have a valid payment method they are navigated to the "enable payments" route to complete KYC checks.
* If they are already KYC'd we will continue whatever action is gated behind the KYC wall.
*
* @param {Event} event
* @param {Event} _event
* @param {String} iouPaymentType
*/
continue(event, iouPaymentType) {
continue(_event, iouPaymentType) {
if (this.state.shouldShowAddPaymentMenu) {
this.setState({shouldShowAddPaymentMenu: false});
return;
}
this.setState({transferBalanceButton: event.nativeEvent.target});
this.setState({transferBalanceButton: this.anchorRef.current});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was required for BaseWalletPage, as transfer balance button contains multiple clickable items so if we used event.nativeEvent.target the popover position used to differ slightly based on where we clicked. See video to see the bug -

Screen.Recording.2023-10-04.at.1.42.55.AM.mov

const isExpenseReport = ReportUtils.isExpenseReport(this.props.iouReport);
const paymentCardList = this.props.fundList || {};

Expand All @@ -110,7 +110,7 @@ class KYCWall extends React.Component {
(!isExpenseReport && !PaymentUtils.hasExpensifyPaymentMethod(paymentCardList, this.props.bankAccountList))
) {
Log.info('[KYC Wallet] User does not have valid payment method');
const clickedElementLocation = getClickedTargetLocation(event.nativeEvent.target);
const clickedElementLocation = getClickedTargetLocation(this.anchorRef.current);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aimane-chnaif if you want we can keep it as this.anchorRef.current || event.nativeEvent.target to be on the safer side. Same for usage above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that for safety

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we apply the change for safety here and also above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aimane-chnaif @marcochavezf done.

const position = this.getAnchorPosition(clickedElementLocation);
this.setPositionAddPaymentMenu(position);
this.setState({
Expand Down Expand Up @@ -142,6 +142,7 @@ class KYCWall extends React.Component {
vertical: this.state.anchorPositionVertical,
horizontal: this.state.anchorPositionHorizontal,
}}
anchorAlignment={this.props.anchorAlignment}
onItemSelected={(item) => {
this.setState({shouldShowAddPaymentMenu: false});
if (item === CONST.PAYMENT_METHODS.BANK_ACCOUNT) {
Expand Down
16 changes: 12 additions & 4 deletions src/components/KYCWall/kycWallPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import _ from 'underscore';
import PropTypes from 'prop-types';
import userWalletPropTypes from '../../pages/EnablePayments/userWalletPropTypes';
import bankAccountPropTypes from '../bankAccountPropTypes';
import cardPropTypes from '../cardPropTypes';
import iouReportPropTypes from '../../pages/iouReportPropTypes';
import reimbursementAccountPropTypes from '../../pages/ReimbursementAccount/ReimbursementAccountDraftPropTypes';
import CONST from '../../CONST';

const propTypes = {
/** Route for the Add Bank Account screen for a given navigation stack */
Expand All @@ -15,9 +17,6 @@ const propTypes = {
/** 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,

Expand All @@ -44,11 +43,16 @@ const propTypes = {

/** The reimbursement account linked to the Workspace */
reimbursementAccount: reimbursementAccountPropTypes,

/** Where the popover should be positioned relative to the anchor points. */
anchorAlignment: PropTypes.shape({
horizontal: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL)),
vertical: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_VERTICAL)),
}),
};

const defaultProps = {
userWallet: {},
popoverPlacement: 'top',
shouldListenForResize: false,
isDisabled: false,
chatReportID: '',
Expand All @@ -58,6 +62,10 @@ const defaultProps = {
reimbursementAccount: {},
addDebitCardRoute: '',
iouReport: {},
anchorAlignment: {
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
},
};

export {propTypes, defaultProps};
4 changes: 4 additions & 0 deletions src/components/MoneyReportHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ function MoneyReportHeader({session, personalDetails, policy, chatReport, report
shouldShowPaymentOptions
style={[styles.pv2]}
formattedAmount={formattedAmount}
anchorAlignment={{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In header button, popover should open below button (anchor should be at top)

horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP,
}}
/>
</View>
)}
Expand Down
4 changes: 4 additions & 0 deletions src/components/ReportActionItem/ReportPreview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ function ReportPreview(props) {
enablePaymentsRoute={ROUTES.ENABLE_PAYMENTS}
addBankAccountRoute={bankAccountRoute}
style={[styles.requestPreviewBox]}
anchorAlignment={{
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In ReportPreview, popover should open above button so anchor is bottom.

}}
/>
)}
</View>
Expand Down
1 change: 1 addition & 0 deletions src/components/SettlementButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ function SettlementButton({
isDisabled={isOffline}
chatReportID={chatReportID}
iouReport={iouReport}
anchorAlignment={anchorAlignment}
>
{(triggerKYCFlow, buttonRef) => (
<ButtonWithDropdownMenu
Expand Down
5 changes: 4 additions & 1 deletion src/pages/settings/Wallet/WalletPage/BaseWalletPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,10 @@ function BaseWalletPage(props) {
enablePaymentsRoute={ROUTES.SETTINGS_ENABLE_PAYMENTS}
addBankAccountRoute={ROUTES.SETTINGS_ADD_BANK_ACCOUNT}
addDebitCardRoute={ROUTES.SETTINGS_ADD_DEBIT_CARD}
popoverPlacement="bottom"
anchorAlignment={{
horizontal: CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT,
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

popover should open below button, so vertical anchor position is top.

}}
>
{(triggerKYCFlow, buttonRef) => (
<MenuItem
Expand Down
Loading