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

Fix: Add Decimal Formatting to Units and Cost - 1529 #1606

Merged

Conversation

hamed-valiollahi
Copy link
Collaborator

This PR adds proper decimal formatting for large numbers in transfers to improve readability.

Closes #1529

Copy link

github-actions bot commented Jan 6, 2025

Frontend Test Results

  1 files  ±0  121 suites  ±0   43s ⏱️ -1s
417 tests ±0  397 ✅ ±0  20 💤 ±0  0 ❌ ±0 
419 runs  ±0  399 ✅ ±0  20 💤 ±0  0 ❌ ±0 

Results for commit ed4374a. ± Comparison against base commit 9ec582f.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 6, 2025

Backend Test Results

503 tests  ±0   503 ✅ ±0   1m 41s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit ed4374a. ± Comparison against base commit 9ec582f.

♻️ This comment has been updated with latest results.

@@ -22,10 +23,12 @@ export const TransferDetails = () => {
register,
control,
watch,
formState: { errors }
formState: { errors },
setValue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need setValue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch! We don't really need it.

@@ -14,6 +14,7 @@ import { Controller, useFormContext } from 'react-hook-form'
import { useTranslation } from 'react-i18next'
import { LabelBox } from './LabelBox'
import { calculateTotalValue } from '@/utils/formatters'
import { NumericFormat } from 'react-number-format'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the formatter formatNumberWithCommas not good for formatting in Transfers? Just thinking on the overhead of updating dependencies when updating React library. If we have an existing formatter, I would prefer using it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see! that was my initial thought to use number formatters to achieve it, but I faced issues with form validation and conversion, which is why I decided to go with a new solution.

Copy link
Collaborator

@areyeslo areyeslo left a comment

Choose a reason for hiding this comment

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

LGTM

@hamed-valiollahi
Copy link
Collaborator Author

LGTM

Thanks, Arturo, for the code review!

@hamed-valiollahi hamed-valiollahi merged commit cfc3bc7 into release-0.2.0 Jan 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LCFS - Add Decimal Formatting to Units and Cost in Transfers for BCeID
2 participants