-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js #35161
Conversation
Hi! So it looks like all of the code in that function you linked ( |
no problem ) |
@akinwale |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@ZhenjaHorbach When I select Send Money from the global create menu, I get a page not found error when I click on the currency. Please check this. send-money-bug.mp4 |
Oh |
A few thoughts about this issue But now we have not Found on IOURequestStepCurrency Because IOURequestStepCurrency requires availability transationID because we use App/src/pages/iou/request/step/IOURequestStepCurrency.js Lines 181 to 186 in 0445cff
But for Because the screen, where we have a problem, will be removed soon So I'm inclined to just wait so as not to cause unnecessary regressionп What do you think about it? |
@tgolen Any thoughts on this? Do we hold until the other PR is merged? The regression that led to this discussion: #35161 (comment) |
Ah yeah, thanks for asking. I think it would be best to just wait on this one for now until that page is removed. |
Can we move forward with this? @akinwale @ZhenjaHorbach @tgolen It's blocking this TS migration 👀 |
@blazejkustra This PR is on hold for #35833. If we go ahead and merge anyway, we'll end up with this regression: #35161 (comment) |
#35833 So I think it's better to wait a little longer |
@ZhenjaHorbach #35833 has been merged. |
Sounds great ) |
Actual PR |
Details
Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js
Fixed Issues
$ #34607
PROPOSAL: #34607 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android_meqiRgGc.mp4
Android: mWeb Chrome
android-web_WXqsnNKV.mp4
iOS: Native
ios_vzi0tXcd.mp4
iOS: mWeb Safari
ios-web_f4uzVwpq.mp4
MacOS: Chrome / Safari
web_y6RDAimu.mp4
MacOS: Desktop
desktop_s6CNeDdK.mp4