-
Notifications
You must be signed in to change notification settings - Fork 546
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 issue 3195 #3201
Fix issue 3195 #3201
Conversation
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.
Idk, I don't have much context on that. Here's my understanding:
- Deprecate
IvyCurrency
- Create a
AvailableCurrenciesUseCase
- Make the CurrencyPicker accept a pre-computed currencies list
I'm not sure about the best path, what you did seems fine. You must handle the same for the account creation flow. If you think that it doesn't break anything we can merge it
@@ -12,5 +13,6 @@ data class SettingsState( | |||
val hideIncome: Boolean, | |||
val treatTransfersAsIncomeExpense: Boolean, | |||
val startDateOfMonth: String, | |||
val progressState: Boolean | |||
val progressState: Boolean, | |||
val manualExchangeRates: List<ExchangeRate> |
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.
This must be ImmutableList from kotlinx collections
@@ -66,6 +67,7 @@ import java.util.Locale | |||
fun CurrencyPicker( | |||
modifier: Modifier = Modifier, | |||
initialSelectedCurrency: IvyCurrency?, | |||
manualExchangeRates: List<ExchangeRate> = listOf(), |
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.
Ideally, the entire currency list (standard + manual) should come from the VM and not he computed in the UI. Also, your fix will work for Settings but not for the account creation flow
Pull Request (PR) Checklist
Please check if your pull request fulfills the following requirements:
main
branch.Screen_recording_20240517_161255.mov
Discussion
Before continue for this path I want to ask your opinion @ILIYANGERMANOV , this is the better way to do it?.
I think maybe the Exchange Rate screen need to be re-write or we need to create a Currency screen.
Does this PR closes any GitHub Issues?
Check Ivy Wallet Issues.
Troubleshooting CI failures ❌
GitHub Actions failing? Read our CI Troubleshooting guide.