Enable user to share community channel #18271
Closed
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
OmarBasem
reviewed
Dec 22, 2023
| channel-id (subs chat-id constants/community-id-length)] | ||
| {:json-rpc/call [{:method "wakuext_shareCommunityChannelURLWithData" | ||
| :params [{:CommunityID community-id :ChannelID channel-id}] | ||
| :on-success #(share/open {:url %}) |
There was a problem hiding this comment.
As per the designs, the share menu needs to have a title as Channel on Status and needs to have a sub-title which is the url (FYI, this is supported only on iOS). You can check the format in status-im.contexts.wallet.account.tabs.about.view (that one doesn't have a sub-title tho)
There was a problem hiding this comment.
I am still waiting on access to the designs. I was unable to find a subtitle option in the docs.
I pushed the changes to use ActivityItemSources. Let me know if they are not matching what the design expects.
ilmotta
requested changes
Dec 26, 2023
|
|
||
| (rf/reg-event-fx :communities/share-community-channel-url-with-data | ||
| (fn [_ [chat-id]] | ||
| (let [community-id (subs chat-id 0 constants/community-id-length) |
There was a problem hiding this comment.
I think this logic to decode the IDs from the chat-id should be a proper function, it would be cleaner.
(require '[legacy.status.im.data-store.chats :as data-store.chats])
(let [{:keys [community-id channel-id]} (data-store.chats/decode-chat-id chat-id)]
...)And also have a simple unit test for the function decode-chat-id.
There was a problem hiding this comment.
moved this to it's own function and have added a test.
| {:json-rpc/call [{:method "wakuext_shareCommunityChannelURLWithData" | ||
| :params [{:CommunityID community-id :ChannelID channel-id}] | ||
| :on-success #(share/open {:url %}) | ||
| :on-error #(log/error "failed to retrieve community channel url with data" %)}]}))) |
There was a problem hiding this comment.
We try to log more descriptive errors, where the second argument to log/error is a map containing whatever could be useful to diagnose the error. Error messages coming from status-go can be very cryptic depending where the failure happened over there. See other calls to log/error, for example:
status-mobile/src/status_im/common/keychain/events.cljs
Lines 162 to 165 in 602b271
We can at least log the chat-id alongside the error.
There was a problem hiding this comment.
updated to log the chat-id along with more error details
| :on-press not-implemented/alert | ||
| :label (i18n/label :t/share-channel)}) | ||
| [chat-id] | ||
| {:icon :i/share |
There was a problem hiding this comment.
Running make lint-fix should fix the indentation issues in the PR.
| channel-id (subs chat-id constants/community-id-length)] | ||
| {:json-rpc/call [{:method "wakuext_shareCommunityChannelURLWithData" | ||
| :params [{:CommunityID community-id :ChannelID channel-id}] | ||
| :on-success #(share/open {:url %}) |
There was a problem hiding this comment.
The default error handling in share/open shouldn't be a no-op (line 7 below), we should at least log errors, but given this is not your PR's responsibility, I suggest we at least call share/open with an on-error callback that logs the error. Swallowing rejections is never a good idea.
status-mobile/src/react_native/share.cljs
Lines 7 to 12 in bec51b7
ajayesivan
reviewed
Jan 1, 2024
| (if platform/ios? | ||
| {:activityItemSources [{:placeholderItem {:type "text" | ||
| :content | ||
| "Channel on Status"} |
There was a problem hiding this comment.
Could you please relocate 'Channel on Status' text to the translations file?
There was a problem hiding this comment.
just to share how this is done. We have the file en.json. Add your key there (anywhere in the file) and it's corresponding translation.
It can then be used with i18n/label function.
e.g require [utils.i18n :as i18n]
and
:content (i18n/label :t/channel-on-status) assuming that you added "channel-on-status" as your translation key in en.json file 👍
J-Son89
reviewed
Jan 2, 2024
| :content %}} | ||
| :linkMetadata {:title | ||
| "Channel on Status"}}]} | ||
| {:title "Channel on Status" |
There was a problem hiding this comment.
I guess these strings should also be i18n labels too 👍
|
@OmarBasem @ilmotta feedback should be addressed if you want to take a second look. |
77% of end-end tests have passedFailed tests (9)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (37)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
@Pau1fitz thanx for the PR. So here we have the same issue on Android that was pointed out in Share wallet PR #18424 (comment) @J-Son89 do we want to fix it separately as well? ISSUE 1 Selected app is opened within Status app tap during sharing of community channel (Android)Tested on Android 12, Samsung Galaxy A 52 Steps:
Actual result: selected app is opened within Status app. User needs to tap system back button to come back to Status app screens.
Expected result: selected app should be opened in separate tab. |
|
So far ISSUE 1 - is the only issue in this PR. |
|
@Pau1fitz thanx! ISSUE 1 is fixed now. PR is ready to be merged. |
|
closed by #18516 |
J-Son89
changed the title
This was referenced Jan 26, 2024
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 #17994
Enable user to share a community channel using a share sheet
Screen.Recording.2023-12-21.at.14.33.46.mov
Figma design
iOS
Android
Testing notes:
Navigate to an individual community channels
click on ellipsis in top right corner
on bottom sheet that opens press share channel
Navigate to list of community channels
Long press on one of them
on bottom sheet that opens press share channel