-
Notifications
You must be signed in to change notification settings - Fork 987
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
feat: dapp connected notification #20905
base: develop
Are you sure you want to change the base?
Conversation
43efe77
to
849ac8f
Compare
Jenkins BuildsClick to see older builds (30)
|
feaa59c
to
77ad8a9
Compare
77ad8a9
to
834a529
Compare
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.
@yqrashawn thank you for the PR! Just a reminder, since we agreed to not include it in the 2.30 release, we should hold on with merging it till after we cut the release branch.
src/status_im/contexts/shell/activity_center/notification/dapp_connection/view.cljs
Outdated
Show resolved
Hide resolved
e714064
to
731f2ab
Compare
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.
LGTM @yqrashawn. I left a minor comment. As @clauxx suggested it's a good idea to hold the merge if it's not critical for 2.30.
src/status_im/contexts/shell/activity_center/notification/dapp_connection/view.cljs
Outdated
Show resolved
Hide resolved
731f2ab
to
1b39c36
Compare
Will request for manual QA after 2.30 |
next up: #20467 (comment) |
1b39c36
to
63acb32
Compare
5a927da
to
82769ee
Compare
82769ee
to
cf6eef2
Compare
@mohsen-ghafouri thank you! Both issues are fixed. One thing that we still need to do is to rebase status go branch status-im/status-go#5616 , then update mobile PR with updated status go commit and re-run e2e tests make sure no issues will leak from status go develop to mobile branch. Could you please ping me up once branches are updated and then I will trigger e2e. Thank you! |
Thank you @pavloburykh, i will ping you once it's ready |
cf6eef2
to
43a7049
Compare
Hey @pavloburykh, I updated branches. please if you can, do another test as status-go side had a slight changes. |
98% of end-end tests have passed
Failed tests (1)Click to expandClass TestWalletMultipleDevice:
Passed tests (50)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestActivityMultipleDevicePR:
|
thanks @mohsen-ghafouri! Tested, PR is ready for merge. |
Thank you for your time and checking this PR @pavloburykh. |
Hey @mohsen-ghafouri! I see that current PR hasn't been merged because corresponding status go PR is blocked. Once it will be unblocked we will need to rebase both branches once again and re run e2e, because there are lot of new commits in go develop since the last test e2e run and we need to make sure they will not brake mobile develop. Also, I see that new conflict has appeared in src/status_im/contexts/shell/activity_center/view.cljs Please ping me up in case PR will need to be re-tested as a result of resolving this conflict. Thank you. |
Hi @pavloburykh, sure. There are ongoing discussions about changes in status-go, and we might implement some updates. I’ll let you know once it’s ready for testing. |
Thanks @mohsen-ghafouri . Pavlo is AFK this week. I took this PR instead. |
Thanks @mariia-skrypnyk, i will let you know once it's ready for test. |
Hi @mohsen-ghafouri ! Any update on this? |
Hi @mariia-skrypnyk, no and i think it can takes time for a while. I will move it to contribution step and remove manual-qa label until it being ready again. |
I think it might be best to park/close this after all, since we picked it up with the assumption that not much more has to be done (as it's not a priority for the current release), but it seems like there are implications on status-go as well and it might take a while to get it in. |
Hey @clauxx, make sense, i just marked as draft until we apply status-go changes. hopefully that won't require any more changes in mobile PR. |
status-im/status-go@4a43b2b...9bc67a0
fixes #20467
Summary
Pressing on the notification to execute its default action will be a separate GH issue.
Review notes
Testing notes
Platforms
Areas that maybe impacted
activity center
Steps to test
status: ready