-
Notifications
You must be signed in to change notification settings - Fork 985
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 QR share screen for community channel #19792
Conversation
Jenkins BuildsClick to see older builds (21)
|
@@ -153,7 +160,7 @@ | |||
{:color colors/white-opa-40 | |||
:container-style style/watched-account-icon}])] | |||
[share-button {:on-press on-share-press}]] | |||
(when (not= share-qr-type :profile) | |||
(when (not (contains? #{:profile :channel} share-qr-type)) |
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.
If it's easy to determine, have a positive if condition, ie. (when (= % :account))
.
If not easy to determine, a more explicit or
is preferable. contains?
feels fragile.
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.
Requested some changes and left some comments not related to your work.
Please feel free to ignore the issues you did not introduce.
(let [params (rf/sub [:get-screen-params]) | ||
;; NOTE(seanstrom): We need to store these screen params for when the modal closes | ||
;; because the screen params will be cleared. | ||
{:keys [url chat-id]} @(rn/use-ref-atom params) |
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.
Since the screen params are present in re-frame state, can't we just fetch it from there whenever we need it ?
Why create an extra copy? What's the constraint here ?
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.
The screen params are stored and then cleared from re-frame state when this view is animated to a closed state. So what will happen is that the url
and chat-id
will be set to nil
, and this will cause a visual glitch with the QR code for a brief moment.
The above code is trying to prevent this visual glitch by keeping a local copy of the screen params. This way when the screen params are cleared while navigating, the QR code will still visualised with the intended url
value.
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.
Does this apply only to this QR code (the visual glitch) or all QR codes?
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.
Good question, I think most other QR codes are unaffected because we don't the screen-params for passing identifiers, they're sorta implicitly in scope for things like wallet accounts or contacts. But communities and community channels do not always have their identifiers implicitly in scope.
For example, we can navigate directly from the home screen to a community QR code, which leads to us passing the community-id through the screen params manually.
I've updated the PR to include a fix for community QR codes as well 🙏
window-width (rf/sub [:dimensions/window-width])] | ||
on-share-community-channel (rn/use-callback | ||
#(rf/dispatch | ||
[:communities/share-community-channel-url-with-data |
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 is a navigate back somewhere in this component. That can be extracted to the let bind and wrapped in a use-callback
.
Not related to your code, but a simple housekeeping win.
src/status_im/contexts/communities/actions/share_community_channel/view.cljs
Outdated
Show resolved
Hide resolved
@@ -16,7 +16,8 @@ | |||
:options [{:key :profile} | |||
{:key :wallet} | |||
{:key :saved-address} | |||
{:key :watched-address}]}]) | |||
{:key :watched-address} | |||
{:key :channel}]}]) | |||
|
|||
(def possible-networks [:ethereum :optimism :arbitrum :myNet]) |
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.
what is :myNet
🤔
After yesterday's conversation on Marking as approved as none of my comments are related to your work directly. |
b08666d
to
fb3c3d7
Compare
90% of end-end tests have passed
Failed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (47)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
|
Hi @seanstrom !🙌 I am moving back your PR to the corresponding column as it doesn't meet the requirements outlined here: Please, make sure your PR contains all necessary labels + information to PR's description. |
@mariia-skrypnyk sorry about that 🙏 |
96% of end-end tests have passed
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (50)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestActivityMultipleDevicePRTwo:
|
@seanstrom yes, it's a tiny issue but I'll check it as PR queue is not so big at the moment. |
fb3c3d7
to
15abe80
Compare
@seanstrom your fix is checked! |
…h nil while closing the share-qr modal
15abe80
to
98babe4
Compare
* tweak: add support for displaying channel qr codes with quo/share-qr-code component * chore: add channel qr-code example to quo preview components * tweak: integrate common/qr-code component for sharing community channel qr-code * tweak: add support for showing community channel avatar in share-qr-code header * tweak: hide wallet tabs for channel qr-code * tweak: integrate share handler for community channel qr-code * fix: prevent accidentally rendering the community channel qr-code with nil while closing the share-qr modal * tidy: extract navigate-back dispatch function * tidy: use styles namespace * tidy: use case statement to determine which qr-code components have a header * tweak: check for share-qr-types that support headers * tidy: extract navigate-back into static function * fix: prevent community qr-code from glitching when closing modal
fixes #19608
Summary
Platforms
Areas that maybe impacted
Functional
Steps to test
Reproduction steps are described in #19608
Before and after screenshots comparison
Before
Screen.Recording.2024-04-25.at.10.22.22.mov
After
Screen.Recording.2024-04-25.at.10.21.28.mov
status: ready