-
Notifications
You must be signed in to change notification settings - Fork 984
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
[#20720] feat: add ability to set currency from account Settings #20772
Conversation
Jenkins BuildsClick to see older builds (83)
|
2206717
to
767e2c9
Compare
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.
Nice work! @mohsen-ghafouri 🙌
src/status_im/contexts/settings/language_and_currency/currency/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/currency/utils.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/currency/view.cljs
Outdated
Show resolved
Hide resolved
d3dad4e
to
746d134
Compare
src/status_im/contexts/settings/language_and_currency/data_store.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/events.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/data_store.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/events.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/currency/utils.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/currency/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/currency/view.cljs
Outdated
Show resolved
Hide resolved
0a0e7ef
to
a380255
Compare
46953a5
to
7a428c2
Compare
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.
src/status_im/contexts/settings/language_and_currency/data_store.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/data_store.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/settings/language_and_currency/events.cljs
Outdated
Show resolved
Hide resolved
93ed153
to
081634a
Compare
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.
Thank you so much for your PR and for addressing the comments!
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.
🙌
25% of end-end tests have passed
Failed tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletMultipleDevice:
Passed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
|
Yes @mohsen-ghafouri , builds appeared but branch has conflicts. Also wanted to ask regarding logic: should I be able to change currency by tap on check box here? IMG_2025.MP4I have an issue with not working sync with desktop app so I am not able to check currency change right now. |
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
@mariia-skrypnyk please, also retest #20694 just in case and close if it doesn't make sense anymore, thank you! |
addaf10
to
9c0a126
Compare
Sorry @mariia-skrypnyk, it was due to this changes #20916 that we hardcoded profile currency to usd, now it should be fine. |
Thanks @mohsen-ghafouri ! Want to take a look but see iOS build is not ok. |
Thanks @mohsen-ghafouri for fix of currency setting from Profile. What makes we curious about are:
IMG_2025.MP4I suppose we need to stay on Figma
![]() |
33% of end-end tests have passed
Failed tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestWalletMultipleDevice:
Passed tests (1)Click to expandClass TestCommunityOneDeviceMerged:
|
Thanks @mariia-skrypnyk for spotting this, you are right, i will fix the auto redirect. |
@mariia-skrypnyk Done. |
Thanks @mohsen-ghafouri ! PR can be merged! |
Thank you @mariia-skrypnyk for your time, @smohamedjavid if all good, can we merge your status-go changes? |
@mohsen-ghafouri - merged the status-go changes and tagged the commit ( |
fixes #20720
fixes #20694
Summary
Add ability to set currency from account Settings.
Areas that maybe impacted
Steps to test
Result
Mobile
Simulator.Screen.Recording.-.iPhone.13.-.2024-07-17.at.23.35.48.mp4
Mobile and Desktop syncing
Screen.Recording.2024-07-17.at.23.46.41.mov
status: ready
status go PR status-im/status-go#5541