-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Plaid popup continue to open from resume app #29927
Plaid popup continue to open from resume app #29927
Conversation
@@ -23,6 +24,10 @@ function PlaidLink(props) { | |||
}, | |||
}); | |||
|
|||
return () => { | |||
dismissLink(); |
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 see this is beyond the proposal. What does this fix?
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.
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.
Ah ok, but what does this fix exactly?
What issue do you have currently in production app? (without calling dismissLink
on unmount)
This is good fix but just to confirm.
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.
when the component is destroyed if the plaid popup opens means it will close the popup.
in the current issue was popup is opened repeatedly after resuming the app
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.
ok so with this fix only, duplicated popup bug is replaced with repeatedly close & open popup 2 times.
So only 1 instance exists, which is a little better than original bug. But still we should fix this for better user experience which is the reason for introducing new Onyx key
Thanks for the explanation.
Screen.Recording.2023-10-19.at.11.24.10.PM.mov
Android bug: app restarts when open from launch icon (only on plaid screen) Screen.Recording.2023-10-19.at.11.59.38.PM.mov |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
@tgolen changes done |
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.
Thank you for the changes. Sorry, I was OOO this week.
@tgolen looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Hm, that's weird. I think all the tests were passing. The last commit shows passing tests... Maybe just a glitch in the matrix. |
🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.93-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀
|
🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.94-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.94-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.94-2 🚀
|
Details
fixed open the plaid popup when resume the app
Fixed Issues
$ #27616
PROPOSAL: #27616 (comment)
Tests
Offline tests
QA Steps
same as test
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
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)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2023-10-19.at.5.45.22.AM.mov
Android: mWeb Chrome
out.mp4
iOS: Native
out.mp4
iOS: mWeb Safari
out.mp4
MacOS: Chrome / Safari
out.mp4
MacOS: Desktop
out.5.43.34.AM.mp4