-
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 EnableStep.js #8832
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.
Code LGTM but we need comprehensive test/QA steps and screenshots in the description
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.
Code looks good! Still need QA steps and videos for each platform though 😄
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.
code LGTM, but like @stitesExpensify said, we need to improve the QA steps here. They are not testing the functionality we are adding.
I think we have some PRs floating around related to the VBA flow that could be used as a guide. Let us know in #engineering-chat
if you have any questions.
Updated and awaiting another review |
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
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@Justicea83 @marcaaron @stitesExpensify There are no QA steps, so we are good to skip this, right? Let me know if you want us to QA this one |
Updating it real quick |
🚀 Deployed to staging by @marcaaron in version: 1.1.64-0 🚀
|
🚀 Deployed to production by @AndrewGable in version: 1.1.64-0 🚀
|
Details
Fixed Issues
$https://github.com/Expensify/Expensify/issues/208197
Tests
Connect Manually Button
.paypalMeAddress
and rvlbankAccountList
,fundList
,userWallet
andnameValuePairs
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
Connect Manually Button
.paypalMeAddress
and rvlbankAccountList
,fundList
,userWallet
andnameValuePairs
is made via Chrome Dev Tools network tabScreenshots
Web
2022-05-18_13-38-58.mp4
Mobile Web
2022-05-18_13-50-46.mp4
Desktop
084009519
Android
2022-05-18_14-25-46.mp4