-
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
Handle cancelled requests correctly #9101
Handle cancelled requests correctly #9101
Conversation
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, but I'm not super familiar with this code. All you @neil-marcellini
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.
The changes look good, the QA with the transition flow works, and the automated test passes.
🚀 Deployed to staging by @neil-marcellini in version: 1.1.65-4 🚀
|
Hey @Expensify/applauseleads, this PR needs to be tested too I believe. |
@Julesssss something weird happened to this PR, it looks like it was merged to staging 3 days ago but added and mentioned in the Staging Checklist an hour ago 🤔 that's why it has not been QAd yet. |
@marcaaron I checked the PR in staging an looks like it's a pass could you give a quick look to verify it's ok? |
That looks correct and I just tested again and verified the issue has been resolved. Thanks! |
🚀 Deployed to production by @Julesssss in version: 1.1.65-6 🚀
|
cc @neil-marcellini
Details
Introduces better handling for cancelled requests.
Fixed Issues (Related to PR)
#8855
Tests & QA
This is probably best tested in connection with the linked PR above. But I think another test is...
Bad Logs
Good Logs
PR Review Checklist
PR 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)