-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
fix: 8541: Resolve multiple transition. #10456
Conversation
return; | ||
} | ||
|
||
this.props.navigation.goBack(); | ||
this.navigateToBankAccountRoute(); |
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.
Shouldn't this onScreenFocus
be removed as well?
/** | ||
* Navigate Bank Account Route | ||
* | ||
* @param {String} policyID | ||
*/ |
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.
It needs a better comment but I don't have enough context to suggest one. I don't know the difference between workspace bank account route and normal bank account route. @puneetlath Can you please suggest one?
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.
Perhaps something like:
Navigate to the correct bank account route based on the bank account state and type
cc @iwiznia
@dharmik Can you work on the requested changes so that we can finish this PR? |
@parasharrajat Please review updated PR. |
|
094198d
to
43261a5
Compare
Complete the checklist. Commits need to be signed. |
43261a5
to
df9ef29
Compare
@parasharrajat Is there any suggestion or way to make last commit to Verify Signed Commit? |
Try this |
@dharmik Awaiting these commits to be signed. |
Details
Fixed Issues
$ #8541
Tests
tests to something as below -
1 Login to New expensify
2 tap on profile
3 Tap on issue card
4 Tap on connect bank account
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
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** 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
tests to something like this -
1 Login to New expensify
2 tap on profile
3 Tap on issue card
4 Tap on connect bank account
Video
iOS
https://user-images.githubusercontent.com/1510175/179397532-7dc20171-cee1-473a-ac10-900bb67262fa.mov
android
https://drive.google.com/file/d/1RbDU7nGZS_l4SUeyzt7ODePctWD7fnh5/view?usp=sharing
Desktop
https://drive.google.com/file/d/1rxk_eKyBRV3t0CvRaCH3WBeBCaHmDW3l/view?usp=sharing
Web
https://drive.google.com/file/d/1OskYEA2YRwNiApCv_d791yu_gRzkVlFW/view?usp=sharing
Mobile Web
https://drive.google.com/file/d/1JZDMy5PrY0LVKqmP9-HWOOrzgb_nn1tT/view?usp=sharing