[BUG#2882] Improved wallet asset selection #2973

jeluard · 2018-01-09T15:15:48Z

relates #2882

EDIT: Addresses #2974 bug and #2975

Summary:

Steps to test:

Open Status
Navigate wallet tab
Asset selection should open a new screen with a list of relevant tokens (both for send and request)

status: ready

dmitryn · 2018-01-09T15:34:28Z

src/status_im/ui/screens/wallet/components/views.cljs

+            [status-im.ui.components.icons.vector-icons :as vector-icons]
+            [status-im.ui.components.list.views :as list]
+            [status-im.ui.components.toolbar.view :as toolbar]
+            [status-im.ui.components.styles :as components.styles]


Seems like already required at line 6

oskarth

I'm not super comfortable with this big a change to cherry-pick in release, but according to Julien this is all necessary to fix #2974 and some button alignment. Let's please test this extra carefully as it's a big change code-wise, IMO.

oskarth · 2018-01-09T16:03:03Z

src/status_im/ui/screens/wallet/subs.cljs

@@ -36,18 +36,6 @@
          str)
      "...")))

-(reg-sub :portfolio-change


Can you explain why this is no longer needed?

Portfolio changes are not displayed anymore, this was a leftover.

oskarth · 2018-01-09T16:03:37Z

src/status_im/ui/screens/wallet/send/views.cljs

@@ -214,14 +217,7 @@
                             :on-focus       (fn [] (when @scroll (js/setTimeout #(.scrollToEnd @scroll) 100)))
                             :on-change-text #(re-frame/dispatch [:wallet.send/set-and-validate-amount %])}}
            (when modal?


What happens if it isn't modal? Why not disabled true for both?

GH diff makes it difficult to read because 2 views have been swapped. It's actually the same thing but couple lines above.

oskarth · 2018-01-09T16:04:45Z

src/status_im/ui/screens/wallet/request/views.cljs

-                                       :value     (name (or symbol :ETH))}]]]]]
+                           :on-change-text #(re-frame/dispatch [:wallet.request/set-and-validate-amount %])}}]]
+        [react/view wallet.styles/choose-asset-container
+         [components/choose-asset {:type   :request


Why is :on-change #(re-frame/dispatch [:wallet.request/set-symbol (keyword %)]) no longer necessary?

It's done when you select an element from the asset list now.

oskarth · 2018-01-09T16:06:00Z

src/status_im/ui/screens/wallet/navigation.cljs

@@ -16,8 +16,10 @@
  db)

 (defmethod navigation/preload-data! :wallet-request-transaction


Can you explain this change? Why navigate back db and why different dissoc/wallet form?

Always dissoc was a bug preventing to select an asset in request screen.
The key change reflect a update to the app-db to align with new semantic (and reflect what is done in send screen).

oskarth · 2018-01-09T16:07:01Z

src/status_im/ui/screens/wallet/components/views.cljs

+       [react/text {:style styles/label}
+        (i18n/label :t/wallet-asset)]
+       [react/touchable-highlight {:style    styles/asset-container
+                                   :on-press #(re-frame/dispatch [:navigate-to (type->view type)])}


What can type be here?
EDIT: See above, so if unknown type then we throw error, OK

janherich · 2018-01-09T16:08:32Z

src/status_im/ui/screens/wallet/components/views.cljs

+
+(defn- render-token [{:keys [symbol name icon decimals]} balance type]
+  [list/touchable-item  #(do (re-frame/dispatch [(type->handler type) symbol])
+                             (re-frame/dispatch [:navigate-back]))


I don't like this double dispatch very much - instead of double dispatch + special preload logic for :navigate-back (hard to track, on 2 places), we should create one atomic handler for that kind of preset/navigation.

Make a todo and an tech-debt issue that links to it @jeluard let's not delay this merge too much

I agree but didn't want to make too big changes. This mimic what is done for the send screen which share most of the logic.

yenda · 2018-01-09T16:10:46Z

src/status_im/ui/screens/wallet/components/views.cljs

+
+(defn- render-token [{:keys [symbol name icon decimals]} balance type]
+  [list/touchable-item  #(do (re-frame/dispatch [(type->handler type) symbol])
+                             (re-frame/dispatch [:navigate-back]))


Make a todo and an tech-debt issue that links to it @jeluard let's not delay this merge too much

oskarth · 2018-01-09T16:13:21Z

Moving to TO TEST to speed up verification. Can still concurrently review to look for potential bugs.

dmitryn · 2018-01-09T16:19:19Z

@jeluard I'd move screen a lit bit down, too close to the top right now, at least in iOS simulator

oskarth · 2018-01-09T16:25:47Z

@dmitryn @jeluard Agree, looks like 16 vs 8 or 32 vs 16 top padding.

jeluard · 2018-01-09T17:10:54Z

@dmitryn Do you mind checking now?

dmitryn · 2018-01-09T17:44:03Z

@jeluard looks good

oskarth · 2018-01-09T18:23:17Z

Thanks for checking screen on device @dmitryn :)

asemiankevich · 2018-01-10T08:46:35Z

ARTIFACT iOS: https://i.diawi.com/hRGwUd
ARTIFACT Android: https://i.diawi.com/ZPhxxr

asemiankevich · 2018-01-10T09:17:28Z

The fix is totally a new feature implemented. I am checking against the invision and here are the remarks:

No Done button
No question icon
No bottom line
Too many space for Gas limit and Gas price, Amount and Fee
No ~ sign
Cross should be back button instead
Transaction Fee header should be centered

jeluard · 2018-01-10T09:21:42Z

@asemiankevich Those changes are not related to the Transaction fee screen

asemiankevich · 2018-01-10T09:34:22Z

i will log it separately @jeluard 👍

asemiankevich · 2018-01-10T10:50:24Z

Branch: PR-2973
Android: https://i.diawi.com/fJT8RV
iOS: https://i.diawi.com/znjNAZ

null is not an object error shown when sending 0 ETH request from Wallet

TF https://app.testfairy.com/projects/4803622-status/builds/7469607/sessions/1/?accessToken=L2mud/NkboZgJdXjOB6XHNgc63U

asemiankevich · 2018-01-10T13:03:01Z

Branch: PR-2973
Android: https://i.diawi.com/RWtJqy
iOS: https://i.diawi.com/3rjNEY

jeluard requested review from dmitryn, goranjovic, yenda, oskarth, rasom and flexsurfer January 9, 2018 15:15

jeluard force-pushed the feature/#2882 branch from debacfd to 5aaf995 Compare January 9, 2018 15:29

jeluard self-assigned this Jan 9, 2018

dmitryn reviewed Jan 9, 2018

View reviewed changes

oskarth approved these changes Jan 9, 2018

View reviewed changes

oskarth reviewed Jan 9, 2018

View reviewed changes

janherich reviewed Jan 9, 2018

View reviewed changes

goranjovic approved these changes Jan 9, 2018

View reviewed changes

yenda approved these changes Jan 9, 2018

View reviewed changes

oskarth added bug 🔢 high prio labels Jan 9, 2018

jeluard force-pushed the feature/#2882 branch from 5aaf995 to 18a63b5 Compare January 9, 2018 16:17

jeluard force-pushed the feature/#2882 branch from 18a63b5 to 61c89fb Compare January 9, 2018 17:07

dmitryn approved these changes Jan 9, 2018

View reviewed changes

asemiankevich self-assigned this Jan 10, 2018

jeluard force-pushed the feature/#2882 branch from 61c89fb to 5c4bd13 Compare January 10, 2018 10:15

asemiankevich added the Tested - Issues label Jan 10, 2018

[BUG#2882] Improved wallet asset selection

7cb9e09

jeluard force-pushed the feature/#2882 branch from 5c4bd13 to 7cb9e09 Compare January 10, 2018 12:35

asemiankevich removed the Tested - Issues label Jan 10, 2018

oskarth mentioned this pull request Jan 10, 2018

Release 0.9.13 WIP #2919

Closed

29 tasks

asemiankevich added the Tested - OK label Jan 10, 2018

dmitryn mentioned this pull request Jan 10, 2018

Currency style is broken on wallet screen when sending funds in 1x1 chat #2924

Closed

oskarth merged commit 7cb9e09 into develop Jan 10, 2018

oskarth deleted the feature/#2882 branch January 10, 2018 16:39

dmitryn mentioned this pull request Jan 10, 2018

Bug/#2924 fix currency font style on send screen #2993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG#2882] Improved wallet asset selection #2973

[BUG#2882] Improved wallet asset selection #2973

jeluard commented Jan 9, 2018 •

edited by oskarth

Loading

dmitryn Jan 9, 2018

jeluard Jan 9, 2018

oskarth left a comment

oskarth Jan 9, 2018

jeluard Jan 9, 2018

oskarth Jan 9, 2018

jeluard Jan 9, 2018

oskarth Jan 9, 2018

jeluard Jan 9, 2018

oskarth Jan 9, 2018

jeluard Jan 9, 2018

oskarth Jan 9, 2018 •

edited

Loading

janherich Jan 9, 2018

yenda Jan 9, 2018

jeluard Jan 9, 2018

yenda Jan 9, 2018

oskarth commented Jan 9, 2018

dmitryn commented Jan 9, 2018

oskarth commented Jan 9, 2018

jeluard commented Jan 9, 2018

dmitryn commented Jan 9, 2018

oskarth commented Jan 9, 2018

asemiankevich commented Jan 10, 2018

asemiankevich commented Jan 10, 2018

jeluard commented Jan 10, 2018

asemiankevich commented Jan 10, 2018

asemiankevich commented Jan 10, 2018 •

edited

Loading

asemiankevich commented Jan 10, 2018

		@@ -16,8 +16,10 @@
		db)

		(defmethod navigation/preload-data! :wallet-request-transaction

[BUG#2882] Improved wallet asset selection #2973

[BUG#2882] Improved wallet asset selection #2973

Conversation

jeluard commented Jan 9, 2018 • edited by oskarth Loading

Summary:

Steps to test:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarth commented Jan 9, 2018

dmitryn commented Jan 9, 2018

oskarth commented Jan 9, 2018

jeluard commented Jan 9, 2018

dmitryn commented Jan 9, 2018

oskarth commented Jan 9, 2018

asemiankevich commented Jan 10, 2018

asemiankevich commented Jan 10, 2018

jeluard commented Jan 10, 2018

asemiankevich commented Jan 10, 2018

asemiankevich commented Jan 10, 2018 • edited Loading

asemiankevich commented Jan 10, 2018

jeluard commented Jan 9, 2018 •

edited by oskarth

Loading

oskarth Jan 9, 2018 •

edited

Loading

asemiankevich commented Jan 10, 2018 •

edited

Loading