Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Guided Setup Stage 3] Update the Pay w/ Expensify selectors #45264
[Guided Setup Stage 3] Update the Pay w/ Expensify selectors #45264
Changes from 17 commits
15c5210
ecc399a
f0a4845
7cea50d
aa98fd4
2aaa0be
037f433
3f00dc6
29eda5e
75e336d
83d33a1
9ba23dd
29aa7f9
fac02f0
83e830a
8f2ad9b
516aec9
9031e1e
bd147f5
fe9662b
2d83339
89ccca8
178cd3a
fbf9e17
2480834
83d2239
2c62b38
732efbf
74aaacb
5fd2f7e
d631958
7a6a1ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we add completePaymentOnboarding to
App/src/components/AddPaymentMethodMenu.tsx
Line 83 in 4fe86eb
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.
@DylanDylann AddPaymentMethodMenu is implemented in two separate components so I'd have to add the same code twice. How about I create a wrapper function such as this:
And replace the part you've selected with
onPaymentItemSelected(paymentMethod)
?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.
@cdOut You mean we should create a new wrapper function called onPaymentItemSelected and use it in AddPaymentMethodMenu. It's fine to me
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'll do that but it won't change the functionality, just move the code around. You can test it in the meantime.
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.
Let's add a JSDoc explaining what the function does
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 data from BE maybe wrong in some cases. Please add two more condition
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.
Why would the data from backend be incorrect? What cases do you have in mind? Those two
ifs
you've provided are already handled automatically on the backend when creating a new account from an invite link.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.
Yeah I understand your point. The BE returned the wrong data in two cases which is the reason why we already implemented two previous conditions. My suggestion will help our change to be safer and make sure that the logic works well.
Anyway, It is not a bug. I will leave the final decision to you 😄
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'd leave it as is - let's not meddle with the data returned from backend more than we have to for the two cases listed in the design doc