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

Show total balance (pending + available) instead of pending balance #8140

Merged
merged 47 commits into from
Feb 29, 2024
Merged

Show total balance (pending + available) instead of pending balance #8140

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
5772d76
Show total balance (pending + available) instead of pending balance w…
shendy-a8c Feb 6, 2024
3c33736
Remove hard-coded available and pending funds for debugging purposes.
shendy-a8c Feb 7, 2024
98fec2f
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 7, 2024
ac07a52
Add tooltip if total balance is negative.
shendy-a8c Feb 7, 2024
6a6f894
Unit tests.
shendy-a8c Feb 7, 2024
0973d61
Merge branch 'update/8070-total-balance' of github.com:Automattic/woo…
shendy-a8c Feb 7, 2024
cc481cc
Improve changelog.
shendy-a8c Feb 7, 2024
f3a90f0
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 7, 2024
abeeaa7
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 8, 2024
53e5491
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 8, 2024
44ba49f
Update changelog because now we are showing total balance instead of …
shendy-a8c Feb 8, 2024
5793404
Always (not only when pending balance is negative) show total balance…
shendy-a8c Feb 8, 2024
1209e97
Update unit tests.
shendy-a8c Feb 8, 2024
7518b04
Remove sprintf import because it's no longer used.
shendy-a8c Feb 8, 2024
c35e509
Remove unused string of Pending funds.
shendy-a8c Feb 9, 2024
00be47d
Merge branch 'develop' into update/8070-total-balance
haszari Feb 14, 2024
2227bde
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 15, 2024
9d36b8a
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 15, 2024
394ec33
Merge branch 'develop' into update/8070-total-balance
haszari Feb 15, 2024
fccad60
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 16, 2024
01f1773
Update tooltip for total balance and available balance.
shendy-a8c Feb 16, 2024
5ca7f15
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 16, 2024
0be8cf4
Update snapshot.
shendy-a8c Feb 16, 2024
1edca23
Fix unit test and snapshot.
shendy-a8c Feb 16, 2024
005f331
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 21, 2024
4f470c4
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 23, 2024
204ce36
Have TotalBalanceTooltip as its own component.
shendy-a8c Feb 23, 2024
24c18fa
CSS changes to make balance tooltip look more like the design.
shendy-a8c Feb 23, 2024
9106b3d
CSS changes to make balance tooltip look more like the design.
shendy-a8c Feb 24, 2024
bc3036f
CSS changes to make balance tooltip look more like the design.
shendy-a8c Feb 24, 2024
53c9227
BalanceBlockProps should be either TooltipBalanceTooltip or Available…
shendy-a8c Feb 24, 2024
b617834
Make AvailableBalanceTooltip as a separate component.
shendy-a8c Feb 24, 2024
eca0bd2
TotalBalanceTooltip should always have maxWidth 315px.
shendy-a8c Feb 24, 2024
c7550fd
Use TotalBalanceTooltip and AvailableBalanceTooltip in AccountBalances.
shendy-a8c Feb 24, 2024
cb68640
Less bottom margin for notice component in total balance tooltip.
shendy-a8c Feb 24, 2024
5b2a7df
Update unit test.
shendy-a8c Feb 24, 2024
12996e6
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 24, 2024
ed5bfb2
Improve text copy for negative balance in balance tooltips.
shendy-a8c Feb 25, 2024
c9c5587
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 26, 2024
d607985
Remove `BalanceTooltip` and render `ClickTooltip` directly in `TotalB…
shendy-a8c Feb 26, 2024
2ec2a73
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 27, 2024
cce2f32
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 28, 2024
6f64549
Generalize `BalanceBlockProps.tooltip`'s type.
shendy-a8c Feb 28, 2024
dcd7b5f
Remove unused import.
shendy-a8c Feb 28, 2024
0b9ab72
Extract out in-line expression to a `isBalanceNegative` variable.
shendy-a8c Feb 28, 2024
6605b40
Update client/components/account-balances/style.scss
shendy-a8c Feb 28, 2024
b8d822c
Merge branch 'develop' into update/8070-total-balance
shendy-a8c Feb 29, 2024
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
4 changes: 4 additions & 0 deletions changelog/update-8070-total-balance
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: update

On Payments Overview page, show total balance (pending + available) instead of pending balance.
229 changes: 105 additions & 124 deletions client/components/account-balances/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import * as React from 'react';
import { Flex, TabPanel } from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';
import { __ } from '@wordpress/i18n';
shendy-a8c marked this conversation as resolved.
Show resolved Hide resolved
import interpolateComponents from '@automattic/interpolate-components';

/**
Expand Down Expand Up @@ -84,16 +84,16 @@ const AccountBalances: React.FC = () => {
className="wcpay-account-balances__balances"
>
<BalanceBlock
id={ `wcpay-account-balances-${ tab.currencyCode }-available` }
title={ fundLabelStrings.available }
amount={ tab.availableFunds }
id={ `wcpay-account-balances-${ tab.currencyCode }-total` }
title={ fundLabelStrings.total }
amount={ 0 }
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 loading state, total balance will be 0.

currencyCode={ tab.currencyCode }
isLoading
/>
<BalanceBlock
id={ `wcpay-account-balances-${ tab.currencyCode }-pending` }
title={ fundLabelStrings.pending }
amount={ tab.pendingFunds }
id={ `wcpay-account-balances-${ tab.currencyCode }-available` }
title={ fundLabelStrings.available }
amount={ 0 }
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 loading state, available balance is 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

In loading state merchant will see a grey box placeholder, so this 0 is all good (if it shows in UI could lead to questions).

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2024-02-19 at 1 34 05 PM

currencyCode={ tab.currencyCode }
isLoading
/>
Expand Down Expand Up @@ -130,129 +130,110 @@ const AccountBalances: React.FC = () => {
isSelectedCurrencyValid ? selectedCurrency : undefined
}
>
{ ( tab: BalanceTabProps ) => (
<>
<Flex
gap={ 0 }
className="wcpay-account-balances__balances"
>
<BalanceBlock
id={ `wcpay-account-balances-${ tab.currencyCode }-available` }
title={ fundLabelStrings.available }
amount={ tab.availableFunds }
currencyCode={ tab.currencyCode }
tooltip={
<BalanceTooltip
label={ `${ fundLabelStrings.available } tooltip` }
content={
tab.availableFunds < 0
? interpolateComponents( {
mixedString: __(
'{{learnMoreLink}}Learn more{{/learnMoreLink}} about why your account balance may be negative.',
'woocommerce-payments'
),
components: {
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.negativeBalance
}
/>
),
},
} )
: interpolateComponents( {
mixedString: __(
'The amount of funds available to be deposited. {{learnMoreLink}}Learn more.{{/learnMoreLink}}',
'woocommerce-payments'
),
components: {
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.depositSchedule
}
/>
),
},
} )
}
/>
}
/>
<BalanceBlock
id={ `wcpay-account-balances-${ tab.currencyCode }-pending` }
title={ fundLabelStrings.pending }
amount={ tab.pendingFunds }
currencyCode={ tab.currencyCode }
tooltip={
<BalanceTooltip
label={ `${ fundLabelStrings.pending } tooltip` }
content={
tab.pendingFunds < 0
? interpolateComponents( {
mixedString: __(
'{{learnMoreLink}}Learn more{{/learnMoreLink}} about why your account balance may be negative.',
'woocommerce-payments'
{ ( tab: BalanceTabProps ) => {
const totalBalance = tab.availableFunds + tab.pendingFunds;

return (
<>
<Flex
gap={ 0 }
className="wcpay-account-balances__balances"
>
<BalanceBlock
id={ `wcpay-account-balances-${ tab.currencyCode }-total` }
title={ fundLabelStrings.total }
amount={ totalBalance }
currencyCode={ tab.currencyCode }
tooltip={
totalBalance < 0 ? (
<BalanceTooltip
label={ `${ fundLabelStrings.total } tooltip` }
content={ interpolateComponents( {
mixedString: __(
'{{learnMoreLink}}Learn more{{/learnMoreLink}} about why your account balance may be negative.',
'woocommerce-payments'
),
components: {
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.negativeBalance
}
/>
),
components: {
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.negativeBalance
}
/>
),
},
} )
: interpolateComponents( {
mixedString: sprintf(
// Translators: %d is the number of days, e.g. 1, 2, 3, etc.
__(
'The amount of funds still in the %d day pending period. {{learnMoreLink}}Learn more.{{/learnMoreLink}}',
},
} ) }
/>
) : undefined
}
/>
<BalanceBlock
id={ `wcpay-account-balances-${ tab.currencyCode }-available` }
title={ fundLabelStrings.available }
amount={ tab.availableFunds }
currencyCode={ tab.currencyCode }
tooltip={
<BalanceTooltip
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth factoring out these into semantic tooltip components, e.g. TotalBalanceTooltip and AvailableBalanceTooltip. Then if there is some logic to fine-tune the content (e.g. add info about implications of a negative total balance), it's abstracted away from this top-level component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note – BalanceTooltip might not be adding much value, it's effectively a synonym / wrapper for ClickToolTip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be worth factoring out these into semantic tooltip components, e.g. TotalBalanceTooltip and AvailableBalanceTooltip.

Good idea. Will implement once we agree on https://github.com/Automattic/woocommerce-payments/pull/8140/files#r1498209420.

label={ `${ fundLabelStrings.available } tooltip` }
content={
tab.availableFunds < 0
? interpolateComponents( {
mixedString: __(
'{{learnMoreLink}}Learn more{{/learnMoreLink}} about why your account balance may be negative.',
'woocommerce-payments'
),
tab.delayDays
),
components: {
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.depositSchedule
}
/>
components: {
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.negativeBalance
}
/>
),
},
} )
: interpolateComponents( {
mixedString: __(
'The amount of funds available to be deposited. {{learnMoreLink}}Learn more.{{/learnMoreLink}}',
'woocommerce-payments'
),
},
} )
}
/>
}
/>
</Flex>
{ tab.instantBalance && tab.instantBalance.amount > 0 && (
<Flex
gap={ 0 }
className="wcpay-account-balances__instant-deposit"
>
<InstantDepositButton
instantBalance={ tab.instantBalance }
components: {
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.depositSchedule
}
/>
),
},
} )
}
/>
}
/>
</Flex>
) }
</>
) }
{ tab.instantBalance && tab.instantBalance.amount > 0 && (
<Flex
gap={ 0 }
className="wcpay-account-balances__instant-deposit"
>
<InstantDepositButton
instantBalance={ tab.instantBalance }
/>
</Flex>
) }
</>
);
} }
</TabPanel>
);
};
Expand Down
1 change: 1 addition & 0 deletions client/components/account-balances/strings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const greetingStrings = {
export const fundLabelStrings = {
available: __( 'Available funds', 'woocommerce-payments' ),
pending: __( 'Pending funds', 'woocommerce-payments' ),
total: __( 'Total balance', 'woocommerce-payments' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it serving us to have these strings factored out of the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 3 usages in client/components/account-balances/index.tsx so there is benefit of storing it as a 'constant' but not sure having a separate strings.ts is a good design.

I see for other component it's structured that way, having a strings.ts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this pattern is used in our code, but I'm not convinced – to me it makes the code less meaningful to extract the strings (and documentation links, etc). One of the wonderful things about react is you can look at the code/markup and understand what the component looks like and does. Moving strings to constants makes this a little harder, since the reader needs to look up each constant to see what the string is. So when I'm working on components that feel disconnected like this, I consider pulling string back in to make it easier to maintain and more self-contained.

Anyway, I'm just sharing my thoughts – no need to change this if it' working for you :)

};

export const documentationUrls = {
Expand Down
Loading
Loading