-
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
Re-fetch actions when we move from offline to online in IOUModal.js #8873
Conversation
cc: @marcaaron |
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.
Please bring back the PR author checklist, as well as add screenshots showing you tested on each platform
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.
Click on the + button close to the text input
Verify that a GET Command is sent with rvl GetLocalCurrency is made via Chrome Dev Tools network tab
I think we're missing a step here.
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.
Please add back the PR Author Checklist
and also add screenshots showing you tested on each platform
Hey @alex-mechler , please this is a sample PR, that didn't require testing on other platforms(#8817 and #8817), can you please take a look? |
I thought they were required for all PRs, asked in contributor management https://expensify.slack.com/archives/C01SKUP7QR0/p1652738900438809 |
Looks like this is linked to the wrong issue. Should be linked here https://github.com/Expensify/Expensify/issues/208196 |
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.
Changes LGTM and test well
Yeah, they were, but this is a special case if you'll. |
I don't think that I agree @Justicea83. You can still test this on all platforms except for iOS. Here's my PR for reference #8868. Even if you can't see the effects of the refresh, you can make sure that the component doesn't break on all platforms, and make sure that we don't get any weird flickering because of the data refresh |
I also don't agree that this is a special case. This is testable on every platform but iOS, and you can still check to ensure nothing has broken like Brandon mentioned. |
Updated and awaiting another review |
🚀 Deployed to staging by @alex-mechler in version: 1.1.63-1 🚀
|
🚀 Deployed to production by @AndrewGable in version: 1.1.63-2 🚀
|
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/208196
Tests
+
button close to the text inputGetLocalCurrency
is made via Chrome Dev Tools network tabPR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
+
button close to the text inputGetLocalCurrency
is made via Chrome Dev Tools network tabScreenshots
Web
2022-05-18_09-44-09.mp4
Mobile Web
2022-05-18_09-47-02.mp4
Desktop
2022-05-18_09-50-33.mp4
Android
android.mp4