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

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

Merged
merged 47 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from 38 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
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.
21 changes: 13 additions & 8 deletions client/components/account-balances/balance-block.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,30 @@ import * as React from 'react';
*/
import { formatCurrency } from 'wcpay/utils/currency';
import Loadable from 'components/loadable';
import BalanceTooltip from './balance-tooltip';
import {
TotalBalanceTooltip,
AvailableBalanceTooltip,
} from './balance-tooltip';

/**
* BalanceBlockProps
*
* @typedef {Object} BalanceBlockProps
*
* @property {string} id The balance block id. Used to link the title and amount.
* @property {string} title The balance title.
* @property {string} currencyCode Currency code of the balance block.
* @property {React.ReactElement< typeof BalanceTooltip >} tooltip The tooltip element.
* @property {number} [amount] Optional. The balance amount.
* @property {boolean} [isLoading] Optional. Whether the balance block is loading.
* @property {string} id The balance block id. Used to link the title and amount.
* @property {string} title The balance title.
* @property {string} currencyCode Currency code of the balance block.
* @property {React.ReactElement< typeof TotalBalanceTooltip | typeof AvailableBalanceTooltip >} tooltip The tooltip element.
* @property {number} [amount] Optional. The balance amount.
* @property {boolean} [isLoading] Optional. Whether the balance block is loading.
*/
interface BalanceBlockProps {
id: string;
title: string;
currencyCode: string;
tooltip?: React.ReactElement< typeof BalanceTooltip >;
tooltip?: React.ReactElement<
typeof TotalBalanceTooltip | typeof AvailableBalanceTooltip
>;
shendy-a8c marked this conversation as resolved.
Show resolved Hide resolved
amount?: number;
isLoading?: boolean;
}
Expand Down
138 changes: 137 additions & 1 deletion client/components/account-balances/balance-tooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,165 @@
*/
import React from 'react';
import HelpOutlineIcon from 'gridicons/dist/help-outline';
import interpolateComponents from '@automattic/interpolate-components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { ClickTooltip } from 'components/tooltip';
import { documentationUrls, fundLabelStrings } from './strings';
import InlineNotice from 'components/inline-notice';

type BalanceTooltipProps = {
label: string;
content: React.ReactNode;
maxWidth?: string | undefined;
};

type TotalBalanceTooltipProps = {
balance: number;
};

type AvailableBalanceTooltipProps = {
balance: number;
};

const BalanceTooltip: React.FC< BalanceTooltipProps > = ( {
label,
content,
maxWidth,
} ) => {
return (
<ClickTooltip
content={ content }
className="wcpay-account-balances__balances__item__tooltip"
buttonIcon={ <HelpOutlineIcon /> }
buttonLabel={ label }
maxWidth={ maxWidth }
Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting to remove BalanceTooltip entirely, it doesn't add much value. The prop values it provides for would be more explicit directly inline in TotalBalanceTooltip and AvailableBalanceTooltip.

No worries if it stays, not causing problems, but if some future engineer feels constrained by it my advice would be to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I missed that. Good point that BalanceTooltip doesn't add much value. Removed in d607985

/>
);
};

export default BalanceTooltip;
export const TotalBalanceTooltip: React.FC< TotalBalanceTooltipProps > = ( {
balance,
} ) => {
return (
<BalanceTooltip
label={ `${ fundLabelStrings.total } tooltip` }
maxWidth={ '315px' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this maxWidth to ensure the equation (Total balance = Available funds + Pending funds) is not wrapped? Would be good to note that in the code, so future readers know why and can iterate on how we handle that constraint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually prefer for more simple diff if we are OK with narrower tooltip like that.

Hmm, I guess we could render the equation on two lines like this:

Total balance = 
Available funds + Pending funds

Not sure if that's clear enough, curious for thoughts from @rogermattic (and on other ideas below).

Adding maxWidth was necessary because the BalanceTooltip renders ClickTooltip which renders TooltipBase which sets an inline max-width in a function that calculates tooltip position, window dimension etc, so it's pretty complex stuffs to modify.

I see! That's frustrating – an example of where an earlier decision / detail limits our options.

  • Can we undo this ? In your opinion, does TooltipBase need to force a max width in inline style, could it move to css?
  • Can we make the base tooltip width bigger (i.e. make all tooltips wider) to work better for our tooltip?
  • If TooltipBase needs to force a width, could it use minWidth, allowing overriding to make things wider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, it looks good in one line.

Would be good to note that in the code, so future readers know why and can iterate on how we handle that constraint.

Good call to put some description in a comment. Added comment in d607985.

In your opinion, does TooltipBase need to force a max width in inline style, could it move to css?

Can't tell for sure. I see that it's set and used in a useEffect() (ref).

If TooltipBase needs to force a width, could it use minWidth, allowing overriding to make things wider?

IMO the current intention is to not let the tooltip content 'stretches' the tooltip without control. This is more important in narrow screen I think.

Changing TooltipBase needs to be done carefully as it might affect other UI and out of scope of this PR. Created #8269.

Choose a reason for hiding this comment

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

Hmm, I guess we could render the equation on two lines like this:

Total balance = 
Available funds + Pending funds

Not sure if that's clear enough, curious for thoughts from @rogermattic (and on other ideas below).

I prefer it when it renders in a single line, BUT I don't think it is necessary as it reads OK when split into two lines.

content={
<>
<>
{ interpolateComponents( {
mixedString: __(
'{{bold}}Total balance{{/bold}} combines both pending funds (transactions under processing) and available funds (ready for deposit). {{learnMoreLink}}Learn more{{/learnMoreLink}}',
'woocommerce-payments'
),
components: {
bold: <b />,
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.depositSchedule
}
/>
),
},
} ) }
</>
<InlineNotice
className="wcpay-account-balances__balances-total-balance-tooltip-notice"
isDismissible={ false }
>
{ __(
'Total balance = Available funds + Pending funds',
'woocommerce-payments'
) }
</InlineNotice>
<>
{ balance < 0 &&
shendy-a8c marked this conversation as resolved.
Show resolved Hide resolved
interpolateComponents( {
mixedString: __(
'Negative account balance? {{discoverWhyLink}}Discover why.{{/discoverWhyLink}}',
'woocommerce-payments'
),
components: {
discoverWhyLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.negativeBalance
}
/>
),
},
} ) }
</>
</>
}
/>
);
};

export const AvailableBalanceTooltip: React.FC< AvailableBalanceTooltipProps > = ( {
balance,
} ) => {
return (
<BalanceTooltip
label={ `${ fundLabelStrings.available } tooltip` }
maxWidth={ balance < 0 ? '280px' : undefined }
content={
<>
<p>
{ interpolateComponents( {
mixedString: __(
'{{bold}}Available funds{{/bold}} have completed processing and are ready to be deposited into your bank account. {{learnMoreLink}}Learn more{{/learnMoreLink}}',
'woocommerce-payments'
),
components: {
bold: <b />,
learnMoreLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.depositSchedule
}
/>
),
},
} ) }
</p>
<p>
{ balance < 0 &&
interpolateComponents( {
mixedString: __(
'Negative account balance? {{discoverWhyLink}}Discover why.{{/discoverWhyLink}}',
Copy link
Contributor

Choose a reason for hiding this comment

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

To me account balance refers to the total balance, and that linked doc page is primarily about why a merchant's total (aka "account") balance is negative (typically: more refunds or disputes than sales).

I don't think showing this info in the pending balance will cause issues, but I'm not convinced it's necessary. When the pending balance is negative, I think (?) this is usually temporary due to how payouts work (possibly specific to instant?), and we don't explain much about this scenario on the help page.

Curious for thoughts from @csmcneill who has more experience of negative balances in practice!

Copy link
Contributor

Choose a reason for hiding this comment

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

When the pending balance is negative, I think (?) this is usually temporary due to how payouts work (possibly specific to instant?), and we don't explain much about this scenario on the help page.

That's my experience of it. I think that we can also include refunds and disputes in here, too, since those can also make an account go negative temporarily. If a balance is negative for an extended period of time, that's usually because we cannot debit the attached bank account or debit card.

I'm planning on including a new section in our document at https://woo.com/document/woopayments/deposits/deposit-schedule/#total-balance that focuses on the Total balance, what it means, and some nuances to keep an eye out for (e.g., it being lower than the available balance due to funds that are in flight).

'woocommerce-payments'
),
components: {
discoverWhyLink: (
// eslint-disable-next-line jsx-a11y/anchor-has-content
<a
rel="external noopener noreferrer"
target="_blank"
href={
documentationUrls.negativeBalance
}
/>
),
},
} ) }
</p>
</>
}
/>
);
};
Loading
Loading