-
Notifications
You must be signed in to change notification settings - Fork 13
Pm 959 tc finance integration #1098
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
Conversation
title='Est. Payment Fees and Tax Withholding %' | ||
// eslint-disable-next-line max-len | ||
value={`Fee: ${nullToZero(walletDetails.estimatedFees)} USD / Tax Withholding: ${nullToZero(walletDetails.taxWithholdingPercentage)}%`} | ||
title='Est. Payment Fees' |
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.
Consider removing the redundant 'USD' from the value string since the dollar sign '$' already indicates the currency.
{walletDetails.taxForm.isSetupComplete && ( | ||
<InfoRow | ||
title='Est. Tax Withholding %' | ||
value={`${nullToZero(walletDetails.taxWithholdingPercentage)}%`} | ||
action={ |
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.
Ensure that the percentage sign '%' is correctly formatted and displayed in all locales. Consider using a localization library if necessary.
align-items: center; | ||
color: $black-100; | ||
|
||
|
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.
Remove the extra blank line for consistency with the rest of the code.
.taxesFooterRow { | ||
margin-top: $sp-6; | ||
|
||
} |
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.
Consider adding some styling properties to .taxesFooterRow
to ensure it has the intended appearance, as it currently only has a margin-top property.
): void { | ||
const totalAmount = parseFloat(totalAmountStr) |
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.
Consider adding error handling for parseFloat(totalAmountStr)
to ensure it doesn't result in NaN
if totalAmountStr
is not a valid number.
): void { | ||
const totalAmount = parseFloat(totalAmountStr) | ||
const taxWithholdAmount = (parseFloat(nullToZero(walletDetails?.taxWithholdingPercentage ?? '0')) * totalAmount) / 100 |
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.
The calculation for taxWithholdAmount
assumes walletDetails?.taxWithholdingPercentage
is a valid number. Consider adding validation or defaulting to a safe value if it's not a valid number.
): void { | ||
const totalAmount = parseFloat(totalAmountStr) | ||
const taxWithholdAmount = (parseFloat(nullToZero(walletDetails?.taxWithholdingPercentage ?? '0')) * totalAmount) / 100 | ||
const feesAmount = parseFloat(nullToZero(walletDetails?.estimatedFees ?? '0')) |
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.
The feesAmount
calculation uses nullToZero(walletDetails?.estimatedFees ?? '0')
. Ensure nullToZero
handles non-numeric strings appropriately to avoid unexpected results.
<hr /> | ||
<div className={`${styles.summary} body-main-bold`}> | ||
<span>Net amount after fees:</span> | ||
<span>{netAmount.toFixed(2)}</span> |
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.
Consider formatting the netAmount
with a currency symbol for consistency with other amounts displayed.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?