Feature/onboarding setup #8234
Merged
Feature/onboarding setup #8234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (263)
|
yenda
reviewed
May 20, 2019
src/status_im/init/core.cljs
Outdated
| #(sort-by :last-sign-in > %)) | ||
| {:keys [address photo-path name]} (first (selection-fn (vals accounts)))] | ||
| (accounts.login/open-login cofx address photo-path name))))) | ||
| (if true #_(empty? accounts) |
src/status_im/subs.cljs
Outdated
| @@ -1631,3 +1631,10 @@ | |||
| :<- [:search/filter] | |||
| (fn [[chats search-filter]] | |||
| (apply-filter search-filter chats extract-chat-attributes))) | |||
|
|
|||
| ;; INTRO WIZARD | |||
| (re-frame/reg-sub | |||
There was a problem hiding this comment.
top level subs should be put at the top of the file
| [status-im.ui.components.colors :as colors] | ||
| [status-im.ui.components.react :as react])) | ||
|
|
||
| (defn number-view |
There was a problem hiding this comment.
is it the same namepad again?
There was a problem hiding this comment.
Yes, but this will be now in a separate PR, together with PINs logic. Removed from this PR now.
| @@ -7,9 +7,59 @@ | |||
| :padding-horizontal 30}) | |||
|
|
|||
| (def intro-logo-container | |||
| {:flex 1 | |||
| {;:flex 1 | |||
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
d8cf3c1
to
459c898
Compare
This was referenced Jun 2, 2019
|
@siphiuel could you rebase please onto develop |
vitvly
force-pushed
the
feature/onboarding-setup
branch
2 times, most recently
from
5960522
to
26ba87d
Compare
flexsurfer
reviewed
Jun 12, 2019
src/status_im/events.cljs
Outdated
| (handlers/register-handler-fx | ||
| :accounts.create.ui/intro-wizard | ||
| (fn [cofx _] | ||
| (accounts.create/intro-wizard cofx))) |
There was a problem hiding this comment.
there is no need to register handlers , you can use {:events []} in fx/defn , please move all these handlers in core , thx
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
4d71f61
to
e2ce232
Compare
flexsurfer
requested changes
Jun 13, 2019
| @@ -30,21 +31,24 @@ | |||
| (assoc cofx :status (rand-nth statuses/data))) | |||
|
|
|||
| (defn create-account! [password] | |||
| (log/info "#create-account!" password) | |||
| (status/create-account | ||
| password | ||
| #(re-frame/dispatch [:accounts.create.callback/create-account-success (types/json->clj %) password]))) | ||
|
|
||
| ;;;; Handlers | ||
| (defn create-account | ||
| [{:keys [db random-guid-generator] :as cofx}] | ||
| [{:keys [db] :as cofx}] | ||
| (log/info "creating account with password:" (get-in db [:intro-wizard :key-code])) |
|
|
||
| (= step 4) | ||
| {:db (-> db | ||
| (assoc-in [:intro-wizard :stored-key-code] (get-in db [:intro-wizard :key-code])) |
There was a problem hiding this comment.
better to use update :intro-wizard and assoc
There was a problem hiding this comment.
Thanks, yes looks better, fixed.
| (fn [data] | ||
| (-> data | ||
| (dissoc :generating-keys?) | ||
| (assoc :accounts (:accounts result)) |
There was a problem hiding this comment.
you can move all keys under one assoc
There was a problem hiding this comment.
Sorry, so lame, fixed.
| @@ -17,3 +17,13 @@ | |||
|
|
|||
| (def about-title-text | |||
| {:font-size 20}) | |||
|
|
|||
| (def learn-more-title | |||
| {:font-size 17 | |||
There was a problem hiding this comment.
you can use typography instead
There was a problem hiding this comment.
Do you mean i can remove line-height and rely on automatic calculation, or something else?
There was a problem hiding this comment.
i mean we don't use styles for text anymore, only :typography because all labels are standatized so instead of [react/text {:style}] you should use [react/text {:typography}
There was a problem hiding this comment.
Thanks for clarifying, fixed.
| {:image (:intro3 resources/ui) | ||
| :title :intro-title3 | ||
| :text :intro-text3}] window-width] | ||
| #_[react/view {:flex 1}] |
| [{:style (assoc styles/welcome-text-bottom-note :color colors/blue) | ||
| :on-press privacy-policy/open-privacy-policy-link!} | ||
| (i18n/label :t/intro-privacy-policy-note2)]] | ||
| #_[privacy-policy/privacy-policy-button]]])) |
| @@ -1,8 +1,10 @@ | |||
|
|
|||
There was a problem hiding this comment.
how's this related to onboarding?
There was a problem hiding this comment.
Leftover from numpad migration to separate PR, removed.
| @@ -98,6 +98,24 @@ | |||
| {:typography :main-medium | |||
There was a problem hiding this comment.
how's this related to onboarding?
There was a problem hiding this comment.
In no way indeed, my mistake, removed.
| @@ -19,6 +19,9 @@ | |||
| (when on-dismiss | |||
| (clj->js {:cancelable false}))))) | |||
|
|
|||
| (defn vibrate [] | |||
There was a problem hiding this comment.
how's this related to onboarding?
There was a problem hiding this comment.
Passcodes don't match error message on Confirm code screen should trigger vibration as per Figma design.
|
Welcome screen: Generate key: Choose key: Choose storage: Create password: Confirm password: Confirm password error: Enable fingerprint: Enable notifications: |
|
Small screen: Welcome: Generate key: Select account: Choose key storage: Create password: Confirm password: Confirm error: Fingerprint: Notifications: |
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
e2ce232
to
9cb488f
Compare
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
cf4cb38
to
1875271
Compare
flexsurfer
reviewed
Jun 14, 2019
| @@ -145,7 +145,7 @@ | |||
| [vector-icons/icon icon {:color (if selected? colors/blue colors/gray) | |||
| :width 24 :height 24}] | |||
| [react/view {:style {:margin-horizontal 16 :flex 1}} | |||
| [react/view {:style {:margin-top -4}} | |||
| [react/view {:style {}} | |||
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
3f6b943
to
9a60608
Compare
0% of end-end tests have passedFailed tests (44)Click to expand
|
|
@siphiuel would you mind to create a separate task for Fingerprint and notification pieces if they are not in scope of this PR? #8234 (comment) |
|
@asemiankevich created an issue #8533 and referenced it in master issue #8089 |
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
6f30ed6
to
73dae1b
Compare
|
Fixed password input text cutting. |
|
@siphiuel could you rebase and squash commits please |
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
73dae1b
to
b9e84e2
Compare
|
@flexsurfer done |
98% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (43)Click to expand |
100% of end-end tests have passedPassed tests (1) |
|
@Serhy awesome job ! Thank you! |
|
@siphiuel please mind latest commits fixing autotests before merge |
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
4de1655
to
434dd5c
Compare
New onboarding e2e tests updates New onboarding e2e fix 2 Signed-off-by: Vitaliy Vlasov <siphiuel@gmail.com>
vitvly
force-pushed
the
feature/onboarding-setup
branch
from
434dd5c
to
e9fd6e1
Compare
|
Noticed some UI issues in the onboarding setup. As this PR is merged, where would you prefer I report these? One seperate issue, multiple seperate issues, or add to an existing issue?:
Alternatively this can also wait untill we have the illustrations and update in one go. Just want to make sure these don't get lost. Some issues already reported earlier by @errorists but outside of PR scope:
|
|
filed new issue #8551 |
Merged
|
Hey, the UI/UX issues I've spotted: Welcome screens
Choose a key and name screen
Select key storage screen
Create password screen
Enable fingerprint
Enable notifications
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8131
Fixes #8132
Fixes #8134