[HOLD for payment 2021-11-05] Payments - add a fill background to "Add new payment method"

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Navigate to payments
2. Click on "add new payment method"

Expected Result:

There should be a grey background the the payment methods menu is open.

Actual Result:

Background stays white.

Workaround:

None needed, visual issue.

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.1.8-0

Reproducible in staging?: Yes
Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @Santhosh-Sellavel
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1634144802229400

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @Beamanator (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @sylveawong (Design), see these Stack Overflow questions for more details.

[kakajann]

Proposal

1. Pass a new prop isAddPaymentMenuVisible={this.state.shouldShowAddPaymentMenu} to PaymentMethodList component after isLoadingPayments prop
   

   App/src/pages/settings/Payments/PaymentsPage.js

   Lines 127 to 131 in 74b77a8

   	<PaymentMethodList
   	onPress={this.paymentMethodPressed}
   	style={[styles.flex4]}
   	isLoadingPayments={this.props.isLoadingPaymentMethods}
   	/>

2. Small modification in addPaymentMethod object

combinedPaymentMethods.push({
    type: MENU_ITEM,
    title: this.props.translate('paymentMethodList.addPaymentMethod'),
    icon: Plus,
    onPress: e => this.props.onPress(e),
    key: 'addPaymentMethodButton',
    disabled: this.props.isLoadingPayments,
    
    // ↓↓↓ THIS KEY ADDEDD
    wrapperStyle: this.props.isShowAddPaymentMenuVisible && styles.switchInactive,
});

3. Finally add wrapperStyle={item.wrapperStyle} prop to MenuItem component in here:
   

   App/src/pages/settings/Payments/PaymentMethodList.js

   Lines 153 to 162 in 74b77a8

   	<MenuItem
   	onPress={item.onPress}
   	title={item.title}
   	description={item.description}
   	icon={item.icon}
   	key={item.key}
   	disabled={item.disabled}
   	iconHeight={item.iconSize}
   	iconWidth={item.iconSize}
   	/>

And of course I'll add new props to propTypes.

Edit: styles.switchInactive is for demo purposes and it can be changed any background color.

[Beamanator]

Hmm I'd like to get confirmation from @sylveawong that this is actually something we want to implement, before reviewing proposals. If it's something we'll fix / work on, I'll definitely mark this External.

@sylveawong it looks like this issue is proposing to give the "Add payment method" button a gray background when it is selected (a.k.a. when the user is choosing what type of payment method to add). A few questions for you:

1. Do you think this is necessary / useful to add?
2. If yes, what color gray would you recommend we make the background of the "Add payment method button"? Or would you recommend some other way to indicate that the button is "active"?

If you prefer, I can bring up this conversation in the Slack thread where this improvement was proposed 🤷 let me know!

[parasharrajat]

Correct @Beamanator. There are other parts of the app that follow this Design pattern. Add secondary login (add a phone number) on the profile page. If we want to change this here. We should Update that part as well.

[sylveawong]

sorry! going to remove myself and @shawnborton / @michelle-thompson so they can get to this first!

[shawnborton]

I think we can just have it maintain its :hover state when it's actively showing a dropdown. Let me know if that makes sense.

[Beamanator]

Yeah that makes sense - the current styles we apply on hover, we'll now apply whenever it's "active" (when it gets clicked)

[Beamanator]

@parasharrajat I see what you mean, the "Add secondary login" button is quite similar to "Add payment method", but the "Add secondary login" button opens a new route (/settings/addlogin/phone) while "Add payment method" just opens a dropdown. I think it's fine to keep these different for the moment, what do you think @shawnborton ?

[Santhosh-Sellavel]

Proposal

This is the style method we use to set background in MenuItem

App/src/styles/styles.js

Lines 2334 to 2338 in dd9e492

	* @param {String} [buttonState] - One of {'default', 'hovered', 'pressed'}
	* @returns {Object}
	*/
	function getButtonBackgroundColorStyle(buttonState = CONST.BUTTON_STATES.DEFAULT) {
	switch (buttonState) {

We can pass CONST.BUTTON_STATES.ACTIVE and get the style and need to pass it as add new property here

App/src/pages/settings/Payments/PaymentMethodList.js

Lines 130 to 136 in dd9e492

	combinedPaymentMethods.push({
	type: MENU_ITEM,
	title: this.props.translate('paymentMethodList.addPaymentMethod'),
	icon: Plus,
	onPress: e => this.props.onPress(e),
	key: 'addPaymentMethodButton',
	disabled: this.props.isLoadingPayments,

wrapperStyle: this.props.isAddPaymentMenuActive ? [getButtonBackgroundColorStyle(CONST.BUTTON_STATES.ACTIVE)] : [],
iconFill: this.props.isAddPaymentMenuActive ?  getIconFillColor(CONST.BUTTON_STATES.ACTIVE)  : null,

And pass it along below,

App/src/pages/settings/Payments/PaymentMethodList.js

Lines 153 to 155 in a142eb2

	<MenuItem
	onPress={item.onPress}
	title={item.title}

isAddPaymentMenuActive prop will added to PaymentMethodList component.

And pass prop,

isAddPaymentMenuActive={this.state.shouldShowAddPaymentMenu}

along with below lines

App/src/pages/settings/Payments/PaymentsPage.js

Lines 127 to 129 in e69f603

	<PaymentMethodList
	onPress={this.paymentMethodPressed}
	style={[styles.flex4]}

For Button state: CONST.BUTTON_STATES.ACTIVE

For Button state: CONST.BUTTON_STATES.PRESSED

@shawnborton which looks good?

cc: @Beamanator

[kakajann]

Forgot to attach the result for my proposal
Here is result:

Screen.Recording.2021-10-19.at.7.08.49.AM.mov

[MelvinBot]

Triggered auto assignment to @bfitzexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[Beamanator]

Unassigning design team since decision was made here, removing n6-hold since the hold is over! Will review proposals soon

[Beamanator]

@shawnborton I just noticed this button technically has 3 states:

1. Sitting alone
2. Hovered
3. Active (when being clicked)

Here's a vid showing all 3 - which do you prefer keeping when the menu is active? Also do we care about the + icon remaining solid black when the menu is open?:

Screen.Recording.2021-10-19.at.5.12.14.PM.mov

[bfitzexpensify]

Upwork posting

[shawnborton]

Hmm good find @Beamanator I think the :active state probably makes the most sense since the row is kind of "active" while the dropdown is shown. Thoughts?

[Beamanator]

Yeah I agree, I think "active" - also making sure the + sign is black, as it is when being clicked & hovered.

@Santhosh-Sellavel @kakajann can you both please update your proposals to include keeping the + icon black? I think in both of your solutions the background will be darker but the + icon changes back to gray

[Santhosh-Sellavel]

@Beamanator updated now!

[kakajann]

That's how I would solve this anyway) as @Santhosh-Sellavel did. iconFill is simply enough. No advanced logic required

[Beamanator]

Thanks for the swift responses guys!

@Santhosh-Sellavel I will give this job to you, i like your use of existing style methods like getButtonBackgroundColorStyle and getIconFillColor, I also like the prop names you suggested. Please move forward with submitting a PR when you have a chance 👍

• Also please apply to the upwork job

[botify]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.11-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Add Payment Method fill when menu is active #5981

If no regressions arise, payment will be issued on 2021-11-05. 🎊

[mallenexpensify]

Paid @Santhosh-Sellavel today since this falls on a weekend. Included reporting and n6-hold bonuses