Feature/wallet choose address contact 6843 #7263
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (15)
|
|
Would it make sense to do divide this work up into smaller chunks? I.e. smaller PRs that are ready and can be merged, as opposed to one big shebang |
|
@oskarth Unfortunately no. |
|
Hey, @goranjovic good job on the redesign. However, I found small bugs:
But it should work rather like this: starts from 0 and goes to this certain amount right directly without reaching 100%.
|
|
@denis-sharypin Once again I'm grateful to you and your excellent eyesight :) |
|
No probs! Fixes related to the gas popup:
Problem: Solution: What do you think? |
|
Check the example — https://www.dropbox.com/s/6vrzdn8gwvgmvac/2019-01-18%2015.36.45.mp4?dl=0
|
goranjovic
force-pushed
the
feature/wallet-choose-address-contact-6843
branch
from
9420c24
to
1b9bc04
Compare
Jenkins BuildsClick to see older builds (159)
|
|
Hey @goranjovic, amazing stuff, thanks! Almost ready, a few things are left:
|
goranjovic
force-pushed
the
feature/wallet-choose-address-contact-6843
branch
2 times, most recently
from
caf0458
to
755bf1d
Compare
goranjovic
changed the title
|
@denis-sharypin Just saw your last comment. Will update this along with any updates needed from the review. |
| (if error | ||
| ;; ERROR | ||
| (do (utils/show-popup (i18n/label :t/error) | ||
| (if (= (:code error) 5) |
There was a problem hiding this comment.
magic number, maybe move to constant
| (defn extract-qr-code-details [chain qr-uri] | ||
| {:pre [(keyword? chain) (string? qr-uri)]} | ||
| ;; i don't like fetching all tokens here | ||
| (let [{:keys [:wallet/all-tokens]} @re-frame.db/app-db |
There was a problem hiding this comment.
Could you please explain why do you get all-tokens data from @re-frame.db/app-db directly?
There was a problem hiding this comment.
because there was strictly speaking no reason to have it through a sub, since the view didn't need to react on it's change
There was a problem hiding this comment.
this doesn't look good to use app-db directly if you need access to db, better to dispatch an event and get db from event's cofx
There was a problem hiding this comment.
@flexsurfer It must not be a triggered event. re-frame cycles don't work well with the qr scanner component, there can be race conditions. That was the cause of that weird QR error on ios.
In any case there is probably a nicer solution, e.g. passing it as a param from one of the enclosing components.
But out of curiosity, is there any technical reason why you think this is bad, or just that it doesn't look pretty (on which we can agree)?
| (cond-> {:from (ethereum/normalized-address from) | ||
| :to (ethereum/normalized-address to) | ||
| :value (ethereum/int->hex amount) | ||
| :gas (ethereum/int->hex gas) | ||
| :gasPrice (ethereum/int->hex gas-price)} | ||
| :gas (ethereum/int->hex (or gas optimal-gas)) |
There was a problem hiding this comment.
What are those? Does it address #7103 ?
There was a problem hiding this comment.
Yes, in fact it does. It is due to the requirements for the redesigned gas popup.
@dmitryn not at the moment, but if any designs in other screens require that, it would be easy to copy the code up in the hierarchy. |
|
@antdanchenko Not in scope of this PR and something that we can fix separately. Is there a way you can temporarily disable that check? Also, does it work on develop? There have been no changes in Sign message |
|
@goranjovic I can disable it, but in that way we are going to introduce an issue in product + skipped test. Is that necessary? |
|
@antdanchenko |
65% of end-end tests have passedFailed tests (20)Click to expand
Passed tests (37)Click to expand |
…backend and other fixes
goranjovic
force-pushed
the
feature/wallet-choose-address-contact-6843
branch
from
d7593f8
to
786b196
Compare
95% of end-end tests have passedFailed tests (3)Click to expand
Passed tests (53)Click to expand |
|
Closing this PR and leaving the branch, with the intention to scavenge the changes into separate PRs with the following goals:
|
|
Branch kept intentionally. Please do not delete it. If you do delete it, please undo :) |
|
@goranjovic Another set of issues/questions: 2) When open "scan" on "Send to" screen black squares are shown on the corners 3) Scan QR on Android leads to
4) #7541 is also reproducible if you paste amount from clipboard
|
|
@goranjovic for future pr(s) regarding wallet re-design: amount length is limited to 20 chars that is not enough, e.g want to send 121,123456789012345678 but can't type such amount |
|
@churik @annadanchenko Good to know, thanks! |
Signed-off-by: Igor Mandrigin <i@mandrigin.ru>
fixes #6843
Summary:
New wallet send flow screens.
Review notes (optional):
styleproperty where the values are not reusable. Also intentional, to increase readability.Testing notes (optional):
Everything related to Wallet needs to be tested, but primarily:
Platforms (optional)
Areas that maybe impacted (optional)
Functional
status: ready