Fix make default payment #8410
Merged
Fix make default payment #8410
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
parasharrajat
suggested changes
Apr 1, 2022
| @@ -40,6 +40,7 @@ class BasePaymentsPage extends React.Component { | |||
| shouldShowDefaultDeleteMenu: false, | |||
| shouldShowPasswordPrompt: false, | |||
| shouldShowConfirmPopover: false, | |||
| isSelectedPaymentMethodDefault: true, | |||
There was a problem hiding this comment.
Suggested change
| isSelectedPaymentMethodDefault: true, | |
| isSelectedPaymentMethodDefault: false, |
| */ | ||
| paymentMethodPressed(nativeEvent, accountType, account) { | ||
| paymentMethodPressed(nativeEvent, accountType, account, accountDefault) { |
There was a problem hiding this comment.
Suggested change
| paymentMethodPressed(nativeEvent, accountType, account, accountDefault) { | |
| paymentMethodPressed(nativeEvent, accountType, account, isDefault) { |
| @@ -143,6 +145,7 @@ class BasePaymentsPage extends React.Component { | |||
| }; | |||
| } | |||
| this.setState({ | |||
| isSelectedPaymentMethodDefault: !accountDefault, | |||
There was a problem hiding this comment.
Don't you think that variable does not justify the value it stores? Shouldn't it be storing the reversed value?
Suggested change
| isSelectedPaymentMethodDefault: !accountDefault, | |
| isSelectedPaymentMethodDefault: accountDefault, |
luacmartins
requested changes
Apr 1, 2022
|
@luacmartins @parasharrajat I've updated. |
|
Testing it in an hour. |
parasharrajat
suggested changes
Apr 4, 2022
luacmartins
requested changes
Apr 4, 2022
There was a problem hiding this comment.
Nice attention to detail @parasharrajat! Let's fix that extra margin @phivh!
|
@luacmartins @parasharrajat Spacing updated! |
parasharrajat
suggested changes
Apr 5, 2022
| @@ -229,6 +232,7 @@ class BasePaymentsPage extends React.Component { | |||
|
|
|||
| render() { | |||
| const isPayPalMeSelected = this.state.formattedSelectedPaymentMethod.type === CONST.PAYMENT_METHODS.PAYPAL; | |||
| const isShowButtonMakeDefaultPayment = !this.state.isSelectedPaymentMethodDefault && Permissions.canUseWallet(this.props.betas) && !isPayPalMeSelected; | |||
There was a problem hiding this comment.
Suggested change
| const isShowButtonMakeDefaultPayment = !this.state.isSelectedPaymentMethodDefault && Permissions.canUseWallet(this.props.betas) && !isPayPalMeSelected; | |
| const shouldShowMakeDefaultButton = !this.state.isSelectedPaymentMethodDefault && Permissions.canUseWallet(this.props.betas) && !isPayPalMeSelected; |
There was a problem hiding this comment.
Updated name does not match with requested change.
luacmartins
reviewed
Apr 5, 2022
| @@ -229,6 +232,7 @@ class BasePaymentsPage extends React.Component { | |||
|
|
|||
| render() { | |||
| const isPayPalMeSelected = this.state.formattedSelectedPaymentMethod.type === CONST.PAYMENT_METHODS.PAYPAL; | |||
| const shouldShowButtonMakeDefaultPayment = !this.state.isSelectedPaymentMethodDefault && Permissions.canUseWallet(this.props.betas) && !isPayPalMeSelected; | |||
There was a problem hiding this comment.
Small name change:
Suggested change
| const shouldShowButtonMakeDefaultPayment = !this.state.isSelectedPaymentMethodDefault && Permissions.canUseWallet(this.props.betas) && !isPayPalMeSelected; | |
| const shouldShowMakeDefaultButton = !this.state.isSelectedPaymentMethodDefault && Permissions.canUseWallet(this.props.betas) && !isPayPalMeSelected; |
|
@luacmartins @parasharrajat Sorry, it should be good now. |
parasharrajat
approved these changes
Apr 6, 2022
There was a problem hiding this comment.
LGTM
cc: @luacmartins
PR Reviewer Checklist
- I verified the PR has a small number of commits behind
main - I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the testing environment is mentioned in the test steps
- I verified testing steps cover success & fail scenarios (if applicable)
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- I verified there are no console errors related to changes in this PR
- I verified proper code patterns were followed (see Reviewing the code)
- I verified comments were added when the code was not self explanatory
- I verified any copy / text shown in the product was added in all
src/languages/*files (if applicable) - I verified proper naming convention for platform-specific files was followed (if applicable)
- I verified style guidelines were followed
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected) - I verified the UI performance was not affected (the performance is the same than
mainbranch) - If a new component is created I verified that a similar component doesn't exist in the codebase
🎀 👀 🎀 C+ reviewed
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
17 tasks
|
🚀 Deployed to staging by @luacmartins in version: 1.1.52-0 🚀
|
|
🚀 Deployed to production by @roryabraham in version: 1.1.52-0 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Details
Fixed Issues
$ #8248
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
web.mp4
Mobile Web
mWeb.mp4
Desktop
desktopapp.mp4
iOS
ios.mp4
Android
android.mp4