Communities: Share airdrop address in non token-gated community #18189
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 (35)
|
J-Son89
reviewed
Dec 14, 2023
| :description :text | ||
| :action :arrow | ||
| :label :preview | ||
| :label-props {:type :accounts | ||
| :data (take 1 accounts)} | ||
| ;; TODO: Substitute the provided data with the selected airdrop |
There was a problem hiding this comment.
please create an issue and link in the TODO. 👍
There was a problem hiding this comment.
We have the issue already, gonna link it. Good idea!
There was a problem hiding this comment.
I recall there was a discussion about discontinuing the use of TODO comments and relying solely on GitHub issues to track tasks.
There was a problem hiding this comment.
Yeah it's preferable to do that 👌
FFFra
force-pushed
the
communities-share-airdrop-address-in-non-token-gated-community
branch
from
c3dacc4 to
6e42c34
Compare
ajayesivan
reviewed
Dec 15, 2023
| [rn/flat-list | ||
| {:data accounts | ||
| :render-fn render-item | ||
| :key-fn :id}])]) |
There was a problem hiding this comment.
I don't remember seeing an :id in the accounts map. Could you please double check?
| :key-fn :id}])]) | ||
|
|
||
| (defn- airdrop-addresses | ||
| [{:keys [accounts logo-uri community-name]}] |
There was a problem hiding this comment.
In this case, the subscription should be here, @ajayesivan is correct.
If I follow the code correctly, in the on-press event we initialise view:
:on-press #(rf/dispatch
[:show-bottom-sheet
{:content (fn [] [airdrop-addresses/view
{:accounts accounts
:logo-uri (get-in images
[:thumbnail :uri])
:community-name name}])}])
accounts is a subscription, so if anything changes in accounts the on-press event will change.
The problem is that if you already clicked, and the view is displayed already and accounts in the db changes, this change won't be reflect in the UI, unless you close and re-open the sheet, making it effectively non-reactive.
So we should subscribe to accounts in this component, and no need to pass accounts on the on-press event. (things might change once we have a selection etc, but for now, that would be the correct implementation I believe).
Comment on lines
30
to
42
| [quo/text {:size :heading-2 :weight :semi-bold} (i18n/label :t/airdrop-addresses)] | ||
| [quo/button | ||
| {:type :grey | ||
| :size 32 | ||
| :icon-only? true | ||
| :on-press #(not-implemented/alert)} | ||
| :i/info]] | ||
| [quo/context-tag | ||
| {:type :community | ||
| :size 24 | ||
| :community-logo logo-uri | ||
| :community-name community-name | ||
| :container-style {:margin-top 8}}] |
There was a problem hiding this comment.
As you mentioned here, we can use drawer-top component.
cammellos
reviewed
Dec 15, 2023
| @@ -41,6 +42,7 @@ | |||
| :accounts | |||
| vals | |||
| (map #(assoc % :customization-color (:color %))))] | |||
|
|
|||
There was a problem hiding this comment.
@FFFra I know you haven't written this code, but this subscription is way to broad, it's subscribing to the whole wallet and pulling just accounts out.
This means that anything that changes in wallet (regardless of whether :accounts is impacted), it will re-render the whole thing.
All of this should be in a subscription:
accounts (->> (rf/sub [:wallet])
:accounts
vals
(map #(assoc % :customization-color (:color %))))]
it should look something like:
accounts-with-customization-color (rf/sub [:wallet/accounts-with-customization-color]) (or whatever naming, the idea is that all the work should be in a subscription and that should be at least a layer-2 subscription, data manipulation in the view should be kept to a minimum.
As a side node, this is a bit an issue with having nested structs in the database, they can help and they are neater, but it's more prone to error, so we should always be careful when we subscribe to make sure we only subscribe to what is strictly needed for the view and no more.
There was a problem hiding this comment.
I have the same thing in my PR too.
There was a problem hiding this comment.
I can update my PR with a subscription like @cammellos suggested and once that's merged @FFFra can make use of that. Or should we go with a follow-up issue approach?
There was a problem hiding this comment.
up to you, since it's not going to be used and we can merge things quite quickly, either are fine, best to merge as quick as possible since it's all behind a flag, and follow up, to avoid conflicts, but up to you, as long as we don't forget.
There was a problem hiding this comment.
I think followup is the best approach to avoid conflicts. I have created this issue and assigned myself, will raise a PR after this one is merged.
@FFFra you can skip this suggestion for now.
cammellos
reviewed
Dec 15, 2023
| :on-press not-implemented/alert | ||
| :on-press #(rf/dispatch | ||
| [:show-bottom-sheet | ||
| {:content (fn [] [airdrop-addresses/view |
There was a problem hiding this comment.
Because you are passing the data on a on-press, this effectively makes everything in that view non-reactive. I think you should just initialise the view and subscribe within that view, so that if things changes while the view is renderered, things change accordingly.
FFFra
force-pushed
the
communities-share-airdrop-address-in-non-token-gated-community
branch
from
6e42c34 to
45eb276
Compare
|
Hello, team/ I've updated the PR description with the new video so we can compare :) |
FFFra
force-pushed
the
communities-share-airdrop-address-in-non-token-gated-community
branch
from
45eb276 to
0ebb106
Compare
27% of end-end tests have passedFailed tests (32)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (13)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
FFFra
force-pushed
the
communities-share-airdrop-address-in-non-token-gated-community
branch
2 times, most recently
from
525f1c5 to
1467f2e
Compare
ajayesivan
force-pushed
the
communities-share-airdrop-address-in-non-token-gated-community
branch
from
1e33cf3 to
39711b0
Compare
ajayesivan
force-pushed
the
communities-share-airdrop-address-in-non-token-gated-community
branch
2 times, most recently
from
2c08f38 to
ce0c5a1
Compare
4 tasks
71% of end-end tests have passedFailed tests (8)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (34)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
|
|
The changes introduced in this PR are disabled in develop under a flag, so it doesn't require manual qa. I think failed e2e is not related to this PR. @status-im/mobile-qa Could you please check and confirm? |
|
@ajayesivan thank you for the PR. Failed e2e are not PR related. Ready for merge. |
ajayesivan
force-pushed
the
communities-share-airdrop-address-in-non-token-gated-community
branch
from
ce0c5a1 to
9beee9c
Compare
ajayesivan
deleted the
communities-share-airdrop-address-in-non-token-gated-community
branch
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 #18081
Summary
This pull request addresses the first two subtasks of #18081, encompassing:
Figma
Please check changes here:
Before refactor, using bottom-sheet:
Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-12-14.at.13.01.16.mp4
After the refactor, using a new screen:
Simulator.Screen.Recording.-.iPhone.13.-.2023-12-20.at.15.44.07.mp4
Review notes
For now, we are not been pixel perfect. To actually select the address is the next scope.
This feature is currently under a flag that is not enabled in the develop branch, so these changes do not require manual QA.
Platforms
Functional
Steps to test
src/status_im2/config.cljsand setcommunity-accounts-selection-enabled?totrue.status: ready