-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[Refactor] Create new OpenIOUModal action instead of GetLocalCurrency #9840
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
Conversation
luacmartins
left a comment
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.
We also need to remove PersonalDetails.fetchLocalCurrency() here -
Line 228 in dd26e82
| PersonalDetails.fetchLocalCurrency(); |
|
Yep, havent pushed these changes yet, I will most likely hold for your PR to be merged first to handle any kind of potential conflicts! |
|
#9560 is merged! |
|
https://github.com/Expensify/Web-Expensify/pull/34248 is in staging and #9560 is merged 🎉 |
|
npm has a |
|
npm has a |
|
This is ready for a review @luacmartins Thank you in advance 🙌 |
luacmartins
left a comment
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.
LGTM and tests well!
|
Web-E PR is in prod! |
|
Merging then |
|
@mountiny looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
|
Not an emergency, tests were passing |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by @luacmartins in version: 1.1.85-8 🚀
|
Details
Needs to be tested together with https://github.com/Expensify/Web-Expensify/pull/34248, it is currently in staging so HOLDing until in production.
In this PR, we are refactoring the
GetLocalCurrencyAPI call into a new one:OpenIOUModalPage.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/213421
Tests
To test and QA this, you should ideally have some VPN.
You can see the tests in the video below.
Screen.Recording.2022-07-18.at.17.36.29.mov
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
See tests section
Screenshots
This change is only related to API hence testing on all the platforms is not necessary.
Refer to the video linked above.
Web
Mobile Web
Desktop
iOS
Android