Allow user to watch a chosen account [UI] #17781
Conversation
J-Son89
requested review from
briansztamfater,
smohamedjavid,
mmilad75,
ulisesmac and
vkjr
| {:flex-direction :row | ||
| :justify-content :space-around | ||
| :padding-vertical 12 | ||
| :padding-horizontal 20}) |
There was a problem hiding this comment.
hmm will this not break other uses of this component? 🤔 The issue might be on the page itself actually.
I would recommend using the regular button instead on that page instead as I think it's also fine to do. This bottom_actions is mostly for drawers as the ns suggests 👌
| @@ -0,0 +1,59 @@ | |||
| (ns status-im.ui.screens.wallet.address-add-edit.views | |||
There was a problem hiding this comment.
hmm, this looks wrong. Why are we adding code into status-im ns? that is legacy code 👍
All new code should be in status-im2 unless absolutely necessary
Jenkins BuildsClick to see older builds (80)
|
| @@ -1,6 +1,7 @@ | |||
| (ns status-im2.contexts.wallet.address-watch.view | |||
There was a problem hiding this comment.
perhaps the ns status-im2.contexts.wallet.select-address-to-watch.view is clearer here and then
the following page can be
status-im2.contexts.wallet.add-watch-only-account.view
| @@ -13,6 +13,8 @@ | |||
| [utils.i18n :as i18n] | |||
| [utils.re-frame :as rf])) | |||
|
|
|||
| (declare account-cards) | |||
There was a problem hiding this comment.
it's a hack to fix cross-reference issue: new-account points to account-cards before the declaration of the latter — so i'm declaring it beforehand
There was a problem hiding this comment.
ok got it, we can clean this up in a follow up. perhaps it's better to add that to the temp file so it's easier to find this afterwards 👍
There was a problem hiding this comment.
btw this is probably as a re-frame sub in the actual view.
similar to how it's done in create account ->
src/status_im2/contexts/wallet/create_account/view.cljs
I'd say go with the approach in create account page and once we have some watch only accounts added we can update the sub to filter for watch only accounts 👍
|
please revert the icon you added, we already have this. 👍 it seems where you added it is in a legacy folder. |
|
UI looks good! nice work :) |
| @@ -0,0 +1,14 @@ | |||
| (ns status-im.ui.screens.wallet.address-add-edit.utils) | |||
|
|
|||
| (defn random-emoji | |||
There was a problem hiding this comment.
is this needed?? 🤔 This is to get the initial random emoji right?
Perhaps you can align with @smohamedjavid on how to select it from the set he added in his emoji picker work?
| :button-two-type - same as button/button :type" | ||
| :actions - keyword (default nil) - :1-action/:2-actions | ||
| :description - string (default nil) - Description to display below the title | ||
| :button-one-disabled? - bool (default false) - Whether the first button is |
There was a problem hiding this comment.
I don't think the naming is clear enough here. Maybe create and continue would be better
There was a problem hiding this comment.
imo it should be button-one-props and button-two-props and we pass the map along 👍
|
@J-Son89 about the icons: i've checked these folders by hand, they don't have this exact icon (see screenshot). About the location — I'll move it to the correct folder, but we should change the docs, since they're pointing to the legacy folder — i can edit them 
|
| @@ -0,0 +1,14 @@ | |||
| (ns status-im.ui.screens.wallet.address-add-edit.utils) | |||
|
|
|||
| (defn random-emoji | |||
There was a problem hiding this comment.
I believe that emojis should be created more generic since they will be used in a lot of other places as well. Don't we already have then in the project?
There was a problem hiding this comment.
Yes, this can be a generic function to get random emoji from the emoji data set.
The data is clojurized in emoji-data def var under status-im2.contexts.emoji-picker.data ns.
We can pass it directly to the rand-nth macro to get the random emoji. The result would be a map that will have a key named unicode which will have the emoji.
ah was not aware of this, yeah that would be great if you can update the docs :) hmm, not sure why that is. because when I search for it should be being used on the edit (wallet) account page if you check |
| :default-value @input-value}] | ||
| [quo/button | ||
| {:icon-only? true | ||
| :type :outline} :i/scan]] | ||
| [rn/view {:style (style/button-container bottom)} | ||
| [quo/text "[WIP] Bottom Actions"]]]))) | ||
| [quo/bottom-actions |
There was a problem hiding this comment.
btw, here we need customization-color on the button too. on this page it's the users custom color 👍 we have this data available on many pages.
you can get it by the following sub {:keys [customization-color]} (rf/sub [:profile/multiaccount])
There was a problem hiding this comment.
great suggestion! i also used this customization color on the second screen, so the user won't need to touch color picked if he would like to preserve the old color
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
from
d163116
to
c08b989
Compare
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
from
c08b989
to
f69c5aa
Compare
| (def container | ||
| {:flex 1}) | ||
| (def input | ||
| {:margin-right 12 | ||
| :flex 1}) | ||
| (def data-item | ||
| {:margin-horizontal 20 | ||
| :padding-vertical 8 | ||
| :padding-horizontal 12}) | ||
| (defn button-container | ||
| [bottom] | ||
| {:position :absolute | ||
| :bottom (+ bottom 12) | ||
| :left 20 | ||
| :right 20}) |
There was a problem hiding this comment.
Please add a line break between styles
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
2 times, most recently
from
17eab55
to
ab09974
Compare
ISSUE 4 Color pickers do not correspond the designs@tumanov-alex I understand that colorpickers are out of scope. But wallet colorpickers have been recently fixed here #17748 and I would expect them to look correct during creating Watch only account. Actual result: less than half of color picker is hidden by default
Expected result: half of color picker should be hidden by default. This is a screenshot how it looks on Account create screen.
|
ISSUE 5 Continue button is hidden behind active keyboard on IOSReproducing on IOS 16.1, iPhoneX Actual result:
Expected result: Continue button should be located above active keyboard
|
|
@pavloburykh thanks for the review! about issues 1, 2 & 3: this is a UI-only ticket, so these feature aren't expected to be implemented in its scope, but would be added alongside actual backend features. 4 & 5 — great catch, working on it |
in addition to this: ISSUE 6 User's current custom color is selected by default when creating watch only accountActual result: telegram-cloud-document-2-5343968403273039042.mp4Expected result: when creating a regular account the first color picker in a row is selected despite of users custom color. I am not sure which behaviour is correct, it is not clear from Figma which behaviour is correct. Better ask the design team. But I believe that watch only and regular account customisation screens should behave the same. telegram-cloud-document-2-5343968403273039048.mp4 |
ISSUE 7 Wallet icon and emoji picker background do correspond the designsThis issue is common for both watch only and regular account customisation screens The color and opacity seems to be wrong. Expected result: 
Actual result:
|
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
from
5435a30
to
6bf5687
Compare
|
@pavloburykh about the issue # 5 — its a little complex to finish right now, while this pr is blocking @mmilad75, so ive talked to Jamie and he confirmed that we can merge the pr without it for now. But ive fixed issues 4 & #17781 (comment) (lets call it isssue # 4.5 :D) cc @J-Son89 |
yes, just to point the issue me & Ulises are addressing is related to - ISSUE 5 Continue button is hidden behind active keyboard on IOS. ISSUE 6 User's current custom color is selected by default when creating watch only account - As Alex said it would be better to handle this separately. The scope of this pr is mostly to get the structure of the UI sitting right and then we will further integrate all of the appropriate behaviours properly in small follow ups as we need this pr merged to begin connecting the backend etc as well :) ISSUE 7 Wallet icon and emoji picker background do correspond the designs - this should be a follow up imo and handled in a separate issue/ pr 👍 We can create the issues to track all of these details! 👌 |
|
Thank you @tumanov-alex ISSUE 5 Continue button is hidden behind active keyboard on IOSand ISSUE 6 User's current custom color is selected by default when creating watch only accountare fixed
Thank you! Will you create followups to track the rest of the issues? |
|
Yes @tumanov-alex & I can create these issues @pavloburykh! Thanks for catching them! 🙌🏼 |
|
@tumanov-alex @J-Son89 I have messed up with issues numeration (already fixed). |
|
Thanks! 🙏 |
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
from
6bf5687
to
ada5d36
Compare
| @@ -219,6 +220,9 @@ | |||
| :options {:insets {:top? true}} | |||
| :component wallet.swap/asset-selector} | |||
|
|
|||
| {:name :address-to-watch-edit | |||
There was a problem hiding this comment.
@tumanov-alex this should be in status_im2 screens file. please move there before merging 👍
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
from
ada5d36
to
6ff09d1
Compare
✔️ status-mobile/prs/android/PR-17781#26 🔹 ~13 min 🔹 9ac8d4f 🔹 📦 android package |
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
2 times, most recently
from
72ee746
to
358dd47
Compare
Add account creation screen remove icons remove extra utility and create a new one that would use conventional way of getting an emoji fix lint Use button component instead of bottom-actions Provide global customization color to buttons Use conventional approach to extract account name Move to another address Move to another namespace Refactor bottom-actions to have button props in maps Update doc with new icon location Add spaces between styles Work on pr comments Use :on-change-text instead of :on-change for input component Subscribe to :profile/customization-color directly Use bottom button from the create-or-edit-account wrapper Remove extra code Sort requires Move ns to proper fileˆ Fix styles
tumanov-alex
force-pushed
the
17490-watch-account-5
branch
from
358dd47
to
2585a13
Compare
Add account creation screen remove icons remove extra utility and create a new one that would use conventional way of getting an emoji fix lint Use button component instead of bottom-actions Provide global customization color to buttons Use conventional approach to extract account name Move to another address Move to another namespace Refactor bottom-actions to have button props in maps Update doc with new icon location Add spaces between styles Work on pr comments Use :on-change-text instead of :on-change for input component Subscribe to :profile/customization-color directly Use bottom button from the create-or-edit-account wrapper Remove extra code Sort requires Move ns to proper fileˆ Fix styles
implements #17490
Design link
(don't mind broken color picker, it's outside of this pr's scope)
Summary
It's a follow up on this: #17479
I added a "continue" button for the "add address to watch" screen and added another screen where you can edit accounts name, emoji & color.
Steps to test