-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Update BasePaymentsPage to call "OpenPaymentMethods" #10161
Conversation
@@ -201,7 +201,7 @@ class BasePaymentsPage extends React.Component { | |||
} | |||
|
|||
fetchData() { |
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 might just be missing it, but I'm not seeing the use of the onyx key IS_LOADING_PAYMENT_METHODS
here - are we using it?
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.
Its going to be used in KYCWall. Here and another spot.
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 look good overall
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!
🚀 Deployed to staging by @Justicea83 in version: 1.1.87-4 🚀
|
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.
@Justicea83 Not a big deal but I would've preferred to review this before it got merged 😅
const onyxData = { | ||
optimisticData: [ | ||
{ | ||
onyxMethod: 'merge', |
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.
NAB: Looks like we've got a constant for onyxMethod: CONST.ONYX.METHOD.MERGE
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 have a follow up PR for antoher part of this refactor, I'll fix it then
@ctkochan22 We are not able to add bank account as in this step |
🚀 Deployed to production by @yuwenmemon in version: 1.1.87-9 🚀
|
Details
As part of the refactor (Web PR and Auth PR) for
"Get rvl=bankAccountList, fundList, userWallet, nameValuePairs"
, we want to pull only what we need, while also handling any offline cases.Fixed Issues
This covers the BasePaymentsPage https://github.com/Expensify/Expensify/issues/216153
Tests
user_good
pass_good
4000056655665556
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
Web
Shown above
iOS