-
Notifications
You must be signed in to change notification settings - Fork 985
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
[#8348] [Multi-Account] Account overview + individual wallet UI #8425
Conversation
Pull Request Checklist
|
@errorists could you check assets management list rendering on ios, thanks |
Jenkins BuildsClick to see older builds (25)
|
(let [request-command (get-in db [:id->command ["request" #{:personal-chats}]])] | ||
(fx/merge cofx | ||
(chat/start-chat public-key nil) |
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.
Why that?
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.
i just moved this code, i think we want to send a message and . if there is no chat we want to start it
(let [price (get-in prices [symbol (-> currency :code keyword) :price])] | ||
(assoc token | ||
:price price | ||
:value (when (and balance price) |
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.
Maybe value shouldn't be in the map at all when nil
checks:
@errorists
|
thanks @asemiankevich
this is how react navigation works, it's not only in this case
not related to this PR
not in the scope of this PR, will disable it for now
this is how it works, we should discuss this with UX team, but i think you expect message only when you use command in chat, it's kind of weird that when you send tokens from somewhere not from chat you send a message, maybe we could add a checkbox, send a message or smth, but again not in scope of this PR, i didn't change anything related to send tokens in this PR |
@flexsurfer first of all major kudos on getting this done so quickly! Below some observations, 1,2,4,7 need design follow-up. Hope the rest is more actionable.
|
thanks @hesterbruikman |
heya @flexsurfer Account explorer:
Account details:
thanks buddy 🙏 |
@flexsurfer I think that's a good solution. The focus of this PR should be enabling an account explorer and viewing multiple accounts. We can iterate on the other solutions to support big numbers/decimals. wdyt @errorists We do need to use the Currency acronym (USD) instead of symbols ($). As multiple currencies are denoted with the $ symbol. Could you add that after the fiat amount? |
@flexsurfer For the short term sure, I guess we can replace the full token name with the fiat value. Long term what I miss the most here is some indication of how much has the asset's value gone up/down. |
@hesterbruikman like this? |
from where? |
from account cards in account explorer, I recall it was there to allow sending-receiving, wanted to make sure that was still the case |
@errorists it works fine on ios simulator, doesn't it work for you? |
56e6979
to
09715ec
Compare
09715ec
to
30d9aee
Compare
55% of end-end tests have passed
Failed tests (21)Click to expand
Passed tests (26)Click to expand |
30d9aee
to
3a320f0
Compare
:<- [:wallet/visible-tokens-symbols] | ||
(fn [[all-tokens visible-tokens]] | ||
(let [vt-set (set visible-tokens)] | ||
(group-by :custom? (map #(assoc % :checked? (boolean (get vt-set (keyword (:symbol %))))) all-tokens))))) |
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.
Maybe can be replaced with ->>
for better readability.
[react/view {:margin-right 8 :flex-shrink 1} | ||
(cond | ||
(string? accessory) | ||
[react/text {:style styles/accessory-text :number-of-lines 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.
Do we need any ellipsize-mode here?
[react/view {:flex-direction :row :margin-bottom 8 :margin-horizontal 4} | ||
[tab-title state :assets (i18n/label :t/wallet-assets) (= tab :assets)] | ||
[tab-title state :nft (i18n/label :t/wallet-collectibles) (= tab :nft)]] | ||
(if (= tab :assets) |
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.
cond
could work to avoid nested if
.
[react/view {:flex 1 :padding-left 16} | ||
(when-not seed-backed-up? | ||
[react/view {:flex-direction :row :align-items :center} | ||
[react/view {:width 14 :height 14 :background-color colors/gray :border-radius 7 :align-items :center |
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.
Weird formatting.
{:events [:wallet.send/set-symbol]} | ||
[{:keys [db]} symbol] | ||
{:db (-> db |
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.
Using update-in
with assoc
would be more readable.
[{:keys [db]} amount symbol decimals] | ||
(let [{:keys [value error]} (wallet.db/parse-amount amount decimals)] | ||
{:db (-> db |
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
@flexsurfer issue found:
Checks that are passed:
NOTE:
|
@asemiankevich known issue #8254 |
3a320f0
to
881412e
Compare
Thanks for the screenshots @asemiankevich. @flexsurfer Given the conversation on Core UI call just now, I can create a seperate issue for this. What do you think? |
@hesterbruikman should be fixed already, in the latest build |
@flexsurfer I am going to fix tests today, please mind my commits before force push |
https://www.dropbox.com/s/7ugg3cugo174j4m/andrey.gif?dl=0
https://www.dropbox.com/s/1iawojw2i68dphf/andrey2.gif?dl=0 …just adding previews for context 👀 |
Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
881412e
to
26bbac8
Compare
@flexsurfer I noticed Account settings is not included. Do you plan on a seperate PR for that? Next to that these are the todo's I noted for design. Am I missing anything?
cc @guylouis |
fixes #8348
Replaced wallet main screen by account screen
Introduced new accounts explorer screen
Moved all events from wallet ui ns to wallet core
Removed unused wallet screens
For QA:
test all wallet screens
and these two
#8370