[Feature] New Decimal Format Applied In Accounts Tab #3587
Conversation
shamim-emon
force-pushed
the
fix-issue-3574
branch
from
79cd6be
to
2064dbd
Compare
ILIYANGERMANOV
requested review from
suyash01,
paulolima18,
Vishwa-Raghavendra,
govardhankoyaraju,
isaacnguyen0809,
Kuladeep1996,
Aditya137x,
akashs056 and
a team
shamim-emon
force-pushed
the
fix-issue-3574
branch
from
cdf0858
to
d801d81
Compare
@ILIYANGERMANOV updated accordingly |
| val context = LocalContext.current | ||
| val scope = rememberCoroutineScope() | ||
| var formattedAmount by remember { mutableStateOf(amount.toDecimalFormatWithFraction()) } | ||
| scope.launch { |
There was a problem hiding this comment.
Use LaunchedEffect this will result in countless coritines launched when recomposition happens
| return useCase.format(this) | ||
| } | ||
|
|
||
| fun Double.toDecimalFormatWithFraction(fraction: Int = 2): String { |
There was a problem hiding this comment.
Why do we need this? Let's use only the FormatMoneyUseCase. My comment is about fixing its decimal format
There was a problem hiding this comment.
@ILIYANGERMANOV Removed this change. Now we see ".0" in place of ".00" again. Please note that this issue is only limited to preview only. In actual app it works as expected.
I tried to use FormatMoneyUseCase for preview. So, I created a non-suspending version of format() here but in such case we can't get preview where it's used(refer to sc below)
So for preview, going back to normal double to string conversion.
Please let me know your comment on this.
Thanks
There was a problem hiding this comment.
Ahaa, you can plug-in a FormarMoneyUsaCase isntance for previews that works. Ideally, this formatting logic should happen in the VM and not in the UI
There was a problem hiding this comment.
How would you recommend using the formatting(for preview) in concerned composable?
1.Passing the ViewModel function or the formatted value itself through the composable hierarchy to concernd composable?
2.Inject the viewmodel within an extension function( haven’t done it before) and call the method there and do the formatting?
3.Any other suggestion?
There was a problem hiding this comment.
@ILIYANGERMANOV How about now? For preview we only need to make sure to show 2 decimal places right? So, how about using existing extension function Double.format(digits) ?
Please let me know if you see any issue with current implemented approach.
There was a problem hiding this comment.
@shamim-emon the existing function should be deleted at some point. Proper fix:
- Provide the format usecase as composition local
- Provide a format usecsee instance for preview
Goals:
- use only the format usecase
- don't couple the code with legacy code that we need to cleanup
If that doesn't work, the proper but harder solution is to rework the VMs to return String instead of Double for the amount that's already formatted. That must be done at some point for performance reasons
| } | ||
|
|
||
| interface FormatMoneyUseCase { |
There was a problem hiding this comment.
What's the purpose of this interface?
There was a problem hiding this comment.
I wanted to use @Binds instead of @Provides for corresponding dependency. Using @provides now and so removed it.
…snapshots updated)
…snapshots updated)
…c update+ other changes)
shamim-emon
force-pushed
the
fix-issue-3574
branch
from
74aac00
to
24bdfee
Compare
|
I see lots of test screen shots missing proper formatting for the total amount shown at the top. To be exact comma(,) is missing from the amount which previously was there. |
github-actions
bot
added
the
Stale
Pull request (PR) checklist
Please check if your pull request fulfills the following requirements:
Screen recording of your changes (if applicable):
What's changed?
Describe with a few bullets what's new:
Risk factors
What may go wrong if we merge your PR?
In what cases won't your code work?
Does this PR close any GitHub issues? (do not delete)
Troubleshooting GitHub Actions (CI) failures ❌
Pull request checks failing? Read our CI Troubleshooting guide.