-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor(deriv_ui): [DERG-3500] return inputted amount based on the formatter in numpad #624
refactor(deriv_ui): [DERG-3500] return inputted amount based on the formatter in numpad #624
Conversation
(cherry picked from commit 0e679136c30a00b1e91f6ea06328d8e07d153186)
dad45d2
to
1ff0feb
Compare
@@ -40,6 +40,9 @@ class CurrencyDetail { | |||
bool get isStableCurrency => | |||
stableCurrencies.contains(currencyType.toUpperCase()); | |||
|
|||
/// This will give the final amount to do processing based on what type of currency it is like fiat or crypto currency. | |||
double get finalAmount => double.parse(formatter.format(amount)); |
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.
@sagar-deriv Can we name it like formattedAmount
?
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.
Please update the doc comments to reflect the updated name.
@@ -54,9 +57,9 @@ class CurrencyDetail { | |||
/// This will give a specific currency formatter based on what type of currency it is like fiat or crypto currency. | |||
NumberFormat get formatter { | |||
if (isFiat || isStableCurrency) { |
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.
Can we update the getter name isStableCurrency
and the list name stableCurrencies
? Using crypto
instead of stable
would be more accurate, as our currency types are fiat
and crypto
.
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.
yeah, that's better. I will push the changes.
@@ -118,9 +118,9 @@ class ExchangeController extends ChangeNotifier { | |||
/// what user send by default in primaryCurrency when coming to numpad.) | |||
double finalAmount() { |
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.
Could we rename the double finalAmount() { ..
method to formattedAmount()
?
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.
In this case finalAmount
is okay, because this is the final amount returned to the user. I will make the change and push for the other one in the CurrencyDetail
class.
235fe96
to
a37bd58
Compare
a37bd58
to
217bfa9
Compare
Clickup link: https://app.clickup.com/t/20696747/DERG-2500
This PR contains the following changes:
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
@akhil-deriv
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)