Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Show total balance (pending + available) instead of pending balance #8140
Changes from 38 commits
5772d76
3c33736
98fec2f
ac07a52
6a6f894
0973d61
cc481cc
f3a90f0
abeeaa7
53e5491
44ba49f
5793404
1209e97
7518b04
c35e509
00be47d
2227bde
9d36b8a
394ec33
fccad60
01f1773
5ca7f15
0be8cf4
1edca23
005f331
4f470c4
204ce36
24c18fa
9106b3d
bc3036f
53c9227
b617834
eca0bd2
c7550fd
cb68640
5b2a7df
12996e6
ed5bfb2
c9c5587
d607985
2ec2a73
cce2f32
6f64549
dcd7b5f
0b9ab72
6605b40
b8d822c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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 inTotalBalanceTooltip
andAvailableBalanceTooltip
.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.
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 d607985There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
Not sure if that's clear enough, curious for thoughts from @rogermattic (and on other ideas below).
I see! That's frustrating – an example of where an earlier decision / detail limits our options.
TooltipBase
need to force a max width in inline style, could it move to css?TooltipBase
needs to force a width, could it useminWidth
, allowing overriding to make things wider?There was a problem hiding this comment.
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.
Good call to put some description in a comment. Added comment in d607985.
Can't tell for sure. I see that it's set and used in a
useEffect()
(ref).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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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 thetotal
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).