Show total balance (pending + available) instead of pending balance #8140
Conversation
…hen pending funds is negative.
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +273 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
…commerce-payments into update/8070-total-balance
|
Just to note, I also found that available balance can be negative while pending balance is positive. See pdjTHR-2Sz-p2#comment-3983. Currently, for such case, we don't show total balance. |
There was a problem hiding this comment.
I've had a play around with this PR and find it a little jarring that the information presented moves around unexpectedly.
Having Total balance / Available balance sometimes and other times Available balance / Pending balance seems confusing to me.
From the issue it sounds like we should always be presenting Total / Available.
I jumped in and assumed we only want to show total balance and available balance only when pending balance is negative by reading the issue's title:
but now I see that it makes more sense to be read as:
I read it as:
Putting this PR back to |
…pending balance all the time (not only when pending balance is negative).
… and available balance block and not pending balance.
| amount={ tab.availableFunds } | ||
| id={ `wcpay-account-balances-${ tab.currencyCode }-total` } | ||
| title={ fundLabelStrings.total } | ||
| amount={ 0 } |
There was a problem hiding this comment.
In loading state, total balance will be 0.
| amount={ tab.pendingFunds } | ||
| id={ `wcpay-account-balances-${ tab.currencyCode }-available` } | ||
| title={ fundLabelStrings.available } | ||
| amount={ 0 } |
There was a problem hiding this comment.
In loading state, available balance is 0.
There was a problem hiding this comment.
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).
|
Searching woo docs for 'pending', found this page that will need updating after this PR is released. Found this too https://woo.com/document/woopayments/deposits/scheduled-deposit-vs-instant-deposit/ but probably safe to keep. |
|
Hey @shendy-a8c! Thanks for working on those changes; it looks good with @haszari 's feedback and the revised logic for the negative balance case! I made this mockup which would be a way to solve the "Learn more" weirdness :) Do you think this could work? 
|
|
Yes! That looks good @rogermattic. Thank you! |
| } ) => { | ||
| return ( | ||
| <ClickTooltip | ||
| content={ content } | ||
| className="wcpay-account-balances__balances__item__tooltip" | ||
| buttonIcon={ <HelpOutlineIcon /> } | ||
| buttonLabel={ label } | ||
| maxWidth={ maxWidth } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah I missed that. Good point that BalanceTooltip doesn't add much value. Removed in d607985
| { balance < 0 && | ||
| interpolateComponents( { | ||
| mixedString: __( | ||
| 'Negative account balance? {{discoverWhyLink}}Discover why.{{/discoverWhyLink}}', |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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).
| @@ -21,7 +21,7 @@ export const greetingStrings = { | |||
|
|
|||
| export const fundLabelStrings = { | |||
| available: __( 'Available funds', 'woocommerce-payments' ), | |||
| pending: __( 'Pending funds', 'woocommerce-payments' ), | |||
| total: __( 'Total balance', 'woocommerce-payments' ), | |||
There was a problem hiding this comment.
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 :)
| return ( | ||
| <BalanceTooltip | ||
| label={ `${ fundLabelStrings.total } tooltip` } | ||
| maxWidth={ '315px' } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
TooltipBaseneed 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
TooltipBaseneeds to force a width, could it useminWidth, allowing overriding to make things wider?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm, I guess we could render the equation on two lines like this:
Total balance = Available funds + Pending fundsNot 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.
…alanceTooltip` and `AvailableBalanceTooltip`. Add comment as to why `maxWidth is specified.
Thank you for the review, @haszari. I'll mark this PR as 'ready to merge' but will wait for other feedbacks before merging. Any follow up can be done in a separate issue / PR. |
| @@ -73,7 +73,6 @@ | |||
| &__tooltip { | |||
| // Specific styles for the click tooltip variant. | |||
| position: relative; | |||
| max-width: 292px; | |||
There was a problem hiding this comment.
Curious to as to why we moved it from styles to props?
There was a problem hiding this comment.
This CSS is overridden by TooltipBase's in-line CSS, so it's unused and removed. We might set max-width using CSS class instead of in-line but since it will involve changing TooltipBase that is used by other UIs, I created #8269.
Co-authored-by: Eric Jinks <[email protected]>
|
Thank you all for the feedback. I think I have addressed them all. Let me know if there is anything else. I plan to merge this PR today at the latest tomorrow so it can make it before the code-freeze. |
|
FYI: all E2E tests are failing due to #8291. |
…8140) Co-authored-by: Rua Haszard <[email protected]> Co-authored-by: Eric Jinks <[email protected]>
Fixes #8070
Changes proposed in this Pull Request
Before:
After:
Total balance will show tooltip if it is negative.
Testing instructions
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge