Implement context action share community QR code #19700
Conversation
Jenkins BuildsClick to see older builds (57)
|
src/status_im/contexts/communities/actions/share_community/view.cljs
Outdated
Show resolved
Hide resolved
94% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (49)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
|
|
Hello! |
|
@Francesca-G can you please review it? |
94% of end-end tests have passedFailed tests (1)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (49)Click to expandClass TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Hi @ibrkhalil ! The last e2e fail is not related to your work so PR will be moved to the next stage for Design Review. |
| window-width (rf/sub [:dimensions/window-width]) | ||
| {{:keys [thumbnail]} :images | ||
| color :color | ||
| community-name :name} (rf/sub [:communities/community community-id]) |
There was a problem hiding this comment.
@ibrkhalil, I would recommend against subscribing to :communities/community if we can because a community instance is updated very frequently and for numerous reasons that are usually unrelated to what is being rendered on screen. For example, if a timestamp in the community is updated, this will force re-frame to do a lot more work to diff the previous and new community map because :communities/community has the entire community as an input signal (in re-frame's words).
Since it's very common to subscribe to a community name, its thumbnail URI and its color to render community avatars, I created some time ago the sub :communities/for-context-tag. The only difference for you is that you will get this map back. Code in the view will be slightly simpler too.
{:name name
:logo (get-in images [:thumbnail :uri])
:color color}
There was a problem hiding this comment.
I agree, It has exactly the info needed to render the component.
And thank you so much for pointing out the cost of listening to every change on the community map.
It makes total sense to minimize the re-renders.
| community-name :name} (rf/sub [:communities/community community-id]) | ||
| navigate-back (rn/use-callback #(rf/dispatch [:navigate-back])) | ||
| on-press-share (rn/use-callback | ||
| #(rf/dispatch |
There was a problem hiding this comment.
We've been generally using #() only in one-liners or very short functions. It's a readability thing. (fn []) instantly informs the dev that there's no argument to worry about.
There was a problem hiding this comment.
Makes total sense, Updating it to be the (fn [] ...) syntax.
| #(rf/dispatch | ||
| [:open-share | ||
| {:options (if platform/ios? | ||
| {:activityItemSources |
There was a problem hiding this comment.
At some point I think we should better abstract this share API. Nothing fancy, but the current way is rather verbose and forces all of us to copy and paste since nobody can remember from the top of their heads the exact shape of the args to pass. Additionally, the checks for iOS should be abstracted away if we can from the view layer once we develop such abstraction.
There was a problem hiding this comment.
100%, Creating a followup to abstract this later and to make it more reusable.
Copying and pasting lines for no reason is not optimal.
There was a problem hiding this comment.
Issue created here
#19749
There was a problem hiding this comment.
Thanks for taking the time to create the issue @ibrkhalil!
| :t/share-community)}}]} | ||
| {:title (i18n/label :t/share-community) | ||
| :subject (i18n/label :t/share-community) | ||
| :message url |
There was a problem hiding this comment.
It's probably fine to not pass url as a dependency to use-callback here because probably url won't change while this view is mounted. But it's a bit like playing with fire and I prefer to always pass dependencies. One of the pain points of embracing these React hooks...
There was a problem hiding this comment.
Adding it to the dependency array.
| (def qr-top-wrapper | ||
| {:margin 12 | ||
| :margin-bottom 0 | ||
| :flex-direction (:flex-direction flex-direction-row) |
There was a problem hiding this comment.
This is a bit odd. Could be just :flex-direction :row because you're already using a var with the word row in it, so the abstraction to get the flex direction from the var is unnecessary.
There was a problem hiding this comment.
Looks nice!
here's the review with a couple of small issues to fix 🙏
fixes #19645
Summary
Implements context action share community QR code
status: ready