-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Handling display of errors when MakeDefaultPaymentMethod fails #10322
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
npm has a |
1 similar comment
npm has a |
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.
Looks great! Just one NAB comment
// eslint-disable-next-line react/default-props-match-prop-types | ||
userWallet: {}, |
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.
I think we can remove this lint disable, what if we updated the userWalletPropTypes to be export PropTypes.shape
and then we can use check that userwallet: userWalletPropTypes
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.
Not sure if there was a reason behind having userWalletPropTypes as:
export default {
/** User's wallet information */
userWallet: PropTypes.shape({
...
}),
};
vs
export default PropTypes.shape({
...
});
But I've updated to the latter so we can avoid disabling the lint error: eslint-disable-next-line react/default-props-match-prop-types
. Updated all usages of userWalletPropTypes as well.
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.
Great solution! I'm not sure if there was a clear reason tbh. I probably was just in the mood to use a spread operator. This seems like a bester "best practice".
… maria-make-default-payment-method # Conflicts: # src/components/AvatarWithIndicator.js
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.
👍
… maria-make-default-payment-method # Conflicts: # src/libs/actions/PaymentMethods.js
…yment-method # Conflicts: # src/pages/EnablePayments/userWalletPropTypes.js # src/pages/settings/Payments/PaymentMethodList.js
Looks like the previous approvals were dismissed when the base branch changed to main. This is on hold till the web PR goes out to production but all set for re-review, @thienlnam @NikkiWines! |
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.
Sweet, thanks for updating the proptype!
@@ -81,4 +87,7 @@ export default withOnyx({ | |||
cardList: { | |||
key: ONYXKEYS.CARD_LIST, | |||
}, | |||
userWallet: { |
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.
This avatarWithIndicator is going to get so bloated with all these onyx keys 😭
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.
😭 Agreed, not sure if there's a better way?
Off hold! |
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.
✋ 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 staging by @NikkiWines in version: 1.1.89-0 🚀
|
This PR is building off of: #10333
Details
Add red brick road offline error handling for MakeDefaultPaymentMethod
Tested with Web PR: https://github.com/Expensify/Web-Expensify/pull/34545
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/216152
Tests
Make default payment method
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** 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)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** 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
Screenshots
Mobile Web