-
Notifications
You must be signed in to change notification settings - Fork 984
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
Community Request to Join - Show Addresses to share Cards #18123
Conversation
Jenkins BuildsClick to see older builds (12)
|
f640d37
to
f9d1f4f
Compare
[quo/category | ||
{:list-type :settings | ||
:data [{:title (i18n/label :t/join-as-a-member) | ||
:on-press #(print "Show All Addresses Sheet") |
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.
To keep the code consistent, we use js/alert
. But even better would be status-im2.common.not-implemented/alert
.
:label :preview | ||
:label-props {:type | ||
:accounts | ||
:data (subvec account-list 0 1)} |
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.
You can probably use (take 1 account-list)
:label-props {:type | ||
:accounts | ||
:data (subvec account-list 0 1)} | ||
:description-props {:text (:name (account-list 0))}}]}] |
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.
It's dangerous to access vectors like this. At least in Clojure JVM you get index out of bounds if the collection is empty.
Best is (-> account-list first :name)
Edit: In ClojureScript we get:
(let [v []]
(v 0)) ; => Exception: No item 0 in vector of length 0
vals | ||
(map #(assoc % :customization-color (:color %))) | ||
vec)] | ||
(tap> account-list) |
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.
Left over
{:keys [name color images]} (rf/sub [:communities/community id])] | ||
{:keys [name color images]} (rf/sub [:communities/community id]) | ||
{:keys [accounts]} (rf/sub [:wallet]) | ||
account-list (->> accounts |
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.
Do we really need account-list
to be an instance of PersistentVector? I saw that label-props
is used with the Clojure's for
macro, so it can be any sequence.
@ilmotta Thank you for the review. I have updated the code please have another look. |
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.
Thanks @ajayesivan, PR looks good by me :)
[quo/category | ||
{:list-type :settings | ||
:data [{:title (i18n/label :t/join-as-a-member) | ||
:on-press #(not-implemented/alert) |
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.
We can pass the function without wrapping it on an anonymous one.
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.
Awesome work! Just left a few aesthetic comments
:action :arrow | ||
:label :preview | ||
:label-props {:type | ||
:accounts |
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.
Should this be in the same line? :type :accounts
:action :arrow | ||
:label :preview | ||
:label-props {:type | ||
:accounts |
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.
same here
0ff6540
to
a2f83c6
Compare
51% of end-end tests have passed
Failed tests (19)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (25)Click to expandClass TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
a2f83c6
to
4d2bb37
Compare
@status-im/mobile-qa This pull request doesn't necessitate manual testing as the modifications are implemented under a feature flag, currently inactive in the develop branch. Could you please review the e2e results and let me know if any failing tests are related to my changes or if I can proceed with the merge? |
@ajayesivan thank you for the PR! Let me re-run e2e because there are bunch of failures caused by message reliability issues. I will keep you posted. |
82% of end-end tests have passed
Failed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (40)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
@ajayesivan failed e2e are not PR related. Ready for merge. |
84% of end-end tests have passed
Failed tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Expected to fail tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
|
Part of #17977 & #18081
Also added some style fixes along with changes.
Known issue
The account icon is currently 24x24, which does not align with the 32x32 size on Figma. This issue stems from the
settings_item
component, where the size is hardcoded. Modifying that component will necessitate manual QA, so I will address it in a separate PR.Test Note
To test this, change the
community-accounts-selection-enabled?
flag insrc/status_im2/config.cljs
totrue
.This feature is currently under a flag that is not enabled in the develop branch, so these changes do not require manual QA.