-
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
Changes from all commits
c7c6313
b04a545
8a58c9b
e9ff91e
013784a
552638e
dec241d
3f6037f
7b92681
257eb61
560c667
8956b42
98babe4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,38 +8,41 @@ | |
[utils.i18n :as i18n] | ||
[utils.re-frame :as rf])) | ||
|
||
(defn navigate-back [] (rf/dispatch [:navigate-back])) | ||
|
||
(defn view | ||
[] | ||
(fn [] | ||
(let [{:keys [url chat-id]} (rf/sub [:get-screen-params]) | ||
(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) | ||
{:keys [color emoji chat-name]} (rf/sub [:chats/community-channel-ui-details-by-id chat-id]) | ||
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 commentThe 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 Not related to your code, but a simple housekeeping win. |
||
chat-id]) | ||
[chat-id])] | ||
[quo/overlay {:type :shell} | ||
[rn/view | ||
{:style {:padding-top (safe-area/get-top)} | ||
{:style (style/container (safe-area/get-top)) | ||
:key :share-community} | ||
[quo/page-nav | ||
{:icon-name :i/close | ||
:on-press #(rf/dispatch [:navigate-back]) | ||
:on-press navigate-back | ||
:background :blur | ||
:accessibility-label :top-bar}] | ||
[quo/text-combinations | ||
{:container-style style/header-container | ||
:title (i18n/label :t/share-channel)}] | ||
[rn/view {:style style/qr-code-wrapper} | ||
[quo/gradient-cover | ||
{:container-style | ||
(style/gradient-cover-wrapper window-width) | ||
:customization-color color}] | ||
[rn/view | ||
{:style {:padding-vertical 12}} | ||
[qr-codes/qr-code | ||
{:size (style/qr-code-size window-width) | ||
:url url | ||
:avatar :channel | ||
:customization-color color | ||
:emoji emoji | ||
:full-name chat-name}]]] | ||
[qr-codes/share-qr-code | ||
{:type :channel | ||
:qr-data url | ||
:customization-color color | ||
:emoji emoji | ||
:full-name chat-name | ||
:on-share-press on-share-community-channel}]] | ||
[quo/text | ||
{:size :paragraph-2 | ||
:weight :regular | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. what is |
||
|
||
|
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
andchat-id
will be set tonil
, 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 🙏