Fix QR share screen for community channel #19792
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Update: It's not easy to determine an exhaustive list. A negative filter is fine.
shivekkhurana
requested changes
Apr 25, 2024
| (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.
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.
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.
Does this apply only to this QR code (the visual glitch) or all QR codes?
There was a problem hiding this comment.
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.
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]) | |||
|
After yesterday's conversation on Marking as approved as none of my comments are related to your work directly. |
shivekkhurana
approved these changes
Apr 26, 2024
seanstrom
force-pushed
the
seanstrom/fix-channel-qr-code-ui
branch
2 times, most recently
from
b08666d
to
fb3c3d7
Compare
clauxx
approved these changes
Apr 30, 2024
90% of end-end tests have passedFailed 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 passedExpected 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. |
mariia-skrypnyk
force-pushed
the
seanstrom/fix-channel-qr-code-ui
branch
from
fb3c3d7
to
15abe80
Compare
|
@seanstrom your fix is checked! |
…h nil while closing the share-qr modal
seanstrom
force-pushed
the
seanstrom/fix-channel-qr-code-ui
branch
from
15abe80
to
98babe4
Compare
codemaster115
pushed a commit
that referenced
this pull request
Apr 30, 2024
* 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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