Add Red Brick Road Offline Handling to Payments List #10333
Conversation
|
npm has a |
2 similar comments
|
npm has a |
|
npm has a |
|
npm has a |
|
npm has a |
1 similar comment
|
npm has a |
|
npm has a |
thienlnam
changed the title
|
npm has a |
melvin-bot
bot
requested review from
aldo-expensify
and removed request for
a team
|
hmm whats with Melvin repeating this comment: #10333 (comment) |
There was a problem hiding this comment.
Looking good so far! the trick to insert the error in Onyx is very clever and made testing easier.. thanks!
I added the error as suggested and disabled the network. I think the item text should have been strikethrough, but it didn't work:
Maybe you need to do something like @iwiznia did here (add style prop to MenuItem in your case... but that may be a bit confusing since there are already other styles there 🤮 )
Probably we are not checking if we already left the comment, but maybe we should not fix that, because Melvin commented 42 times, but the PR still contains changes in the lock, when it shouldn't 😄 |
| @@ -78,39 +87,6 @@ const defaultProps = { | |||
| ...withCurrentUserPersonalDetailsDefaultProps, | |||
| }; | |||
|
|
|||
| const defaultMenuItems = [ | |||
There was a problem hiding this comment.
The brickRoadIndicator: PaymentMethods.hasPaymentMethodError(props.bankAccountList, props.cardList) ? 'error' : null, needs access to something in props now
|
I think this is missing adding the dot to the avatar, no? |
This seems to be a bug from the cli tools utils where the card gets added to the bank list and the card list like Maria mentioned here #10333 (review) I'll check it though and see if it is also happening to other cards |
Ahh, I see, I'll test it with a fresh account without adding stuff with clitools |
|
Nevermind, found the issue - it was looking for cardID instead of fundID which is why it wouldn't delete But I believe the bug where closing the error adds another error to the bank account of the same number is a bug from the cli tools |
|
You have some conflicts now, can you resolve before I do a final test 🙏 |
|
Should be good now! @aldo-expensify |
There was a problem hiding this comment.
Seems to be working well, tested in web and Android (real device)
To have the errors in Android, I found it easier to send them from the Web API adding these lines
$bankAccountList = array_map(function (array $bankAccount) {
return array_merge($bankAccount, ['pendingAction' => 'delete', 'errors' => ['1' => 'This is an error']]);
}, $bankAccountList);
$fundList = array_map(function (array $fund) {
return array_merge($fund, ['pendingAction' => 'delete', 'errors' => ['1' => 'This is an error']]);
}, $fundList);before construction $onyxData here: https://github.com/Expensify/Web-Expensify/blob/3b9e65f83ed4abe74e8a4bf36a84fe9cd3356f23/lib/BankAccountAPI.php#L962
|
Going to self merge since I've addressed all reviewer comments and don't want to deal with any more merge conflicts - feel free to leave another review and I'll address in another PR |
|
👋 |
|
I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check. We've also updated the item in the checklist with this PR to avoid this issues in the future. |
|
Thanks, @mananjadhav. Could you point out the line(s) of code in this PR that used conditional render with string operands? |
| ); | ||
| </View> | ||
| <View style={[styles.flexRow, styles.menuItemTextContainer, styles.pointerEventsNone]}> | ||
| {props.badgeText && <Badge text={props.badgeText} badgeStyles={[styles.alignSelfCenter, (props.brickRoadIndicator ? styles.mr2 : undefined)]} />} |
There was a problem hiding this comment.
@MariaHCD here is one example. But now when I searched with badgeText it looks like it existed before the PR too (sorry about that, had to link too many PRs here). I'll find the original PR and comment there too.
There was a problem hiding this comment.
All good, thanks for clarifying! It was mostly for my understanding :)
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/221881
Tests
window.Onyx = Onyx;to setup/index.jsOnyx.merge('bankAccountList', {"827591": {pendingAction: 'delete', errors: {1: 'This is an error'}}})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
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** 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
Mobile Web
Desktop
iOS
Android