Onboarding ui touchups #8903
Merged
Onboarding ui touchups #8903
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 (284)
|
|
@siphiuel is it still draft? |
|
@flexsurfer forgot to mark it as ready for review. There are some questions I've had to @andmironov , but otherwise I think it's ready for review. |
vitvly
force-pushed
the
fix/8551-onboarding-ui-touchups
branch
from
945f250 to
00f3fd3
Compare
flexsurfer
approved these changes
Sep 4, 2019
yenda
approved these changes
Sep 4, 2019
29% of end-end tests have passedFailed tests (32)Click to expand
Passed tests (13)Click to expand
|
Please take a look @siphiuel |
|
@asemiankevich I can see FaceID/TouchID screens on emulators https://monosnap.com/file/jPVfioEn9k7uZRYtOywKkrKeJHVkDF (iPhone Xs) and https://monosnap.com/file/eyzscqqyQU2SAeb4ReA0ZB3emAa2KH (iPhone 8), but indeed, not on real iPhone 7 from diawi build. Will check |
andmironov
reviewed
Sep 5, 2019
There was a problem hiding this comment.
Generate a key
- No screen overflow animation between "Choose a key and name" and "Select key storage" screens;
Recovery with seed phrase
- The toolbar is too big. It supposed to have a fixed height, just as on any other screens of the app
- There are some layout issues and i've added some minor design changes
- There's not a top border in the toolbar, just like in the onboarding flow;
- The description text, explaining the number of words the user has to enter is aligned to the bottom and has an 8px margin between it and the toolbar;
- The word count now has "Word count:" prefix. It is invisible when there has been no words entered yet, has a green check icon in a semi-transparent green circle when the word count matches the required number of words and is also aligned to the bottom and has an 8 px bottom margin;
- The error message has one line and does not cover the title;
- The words are vertically aligned in the middle between the screen title and the description text;
-
The above is true for the "re-encrypting with password" screens, which come after the successful seed phrase recovery: top bar, toolbar, alignment;
-
"Re-encrypt your key" button's bottom margins seem not to be 16px which it should (not sure);
-
"Welcome to status" screen has a broken icon in it and is shown not long enough, there should be enough time to read the text on it.
|
@andmironov i'll move Recovery-related stuff into a separate issue #8911, as suggested yesterday #8551 (comment). @andmironov @flexsurfer Regarding the missing animation between screen transitions - we do not use react-navigation inside the wizard now, it implements its own step transitions. Is it critical to implement in this issue, or maybe we can have a separate one for that? |
|
One more thing I keep forgetting is the "Continue syncing" popup which now appears on top of the fingerprint screen and does not make sense to a new user at all. |
|
@yenda @flexsurfer can you please have a look at 6162901 and review if it's ok |
Oops! 😬 I think I am the culprit. Sorry. |
vitvly
force-pushed
the
fix/8551-onboarding-ui-touchups
branch
from
6162901 to
59830ad
Compare
|
@siphiuel yes I think you need to include anything that starts with |
|
thanks @yenda, just tried the latest build after adding |
vitvly
force-pushed
the
fix/8551-onboarding-ui-touchups
branch
from
7778596 to
2718243
Compare
vitvly
force-pushed
the
fix/8551-onboarding-ui-touchups
branch
from
67344b1 to
3c054a4
Compare
yenda
reviewed
Oct 9, 2019
| @@ -197,16 +223,17 @@ | |||
|
|
|||
| [react/text-input {:secure-text-entry true | |||
| :auto-focus true | |||
There was a problem hiding this comment.
Suggested change
| :auto-focus true | |
| :auto-focus auto-focus? |
yenda
reviewed
Oct 9, 2019
| :text-align :center | ||
| :placeholder "" | ||
| :style (styles/password-text-input (- view-width (* 2 horizontal-margin))) | ||
| :on-change-text #(re-frame/dispatch [:intro-wizard/code-symbol-pressed %])}]] | ||
| [react/text {:style (assoc styles/wizard-text :margin-bottom 16)} (i18n/label :t/password-description)]])) | ||
|
|
||
| (defn create-code [{:keys [confirm-failure?] :as wizard-state} view-width] | ||
| (defn create-code [{:keys [confirm-failure? view-width]}] | ||
| [password-container confirm-failure? view-width]) |
There was a problem hiding this comment.
Suggested change
| [password-container confirm-failure? view-width]) | |
| [password-container confirm-failure? view-width true]) |
yenda
reviewed
Oct 9, 2019
| @@ -217,108 +244,357 @@ | |||
| (i18n/label :t/processing)]] | |||
| [password-container confirm-failure? view-width])) | |||
There was a problem hiding this comment.
Suggested change
| [password-container confirm-failure? view-width])) | |
| [password-container confirm-failure? view-width (not processing?)])) |
There was a problem hiding this comment.
well it doesn't work like this because processing? is false again but maybe there's a solution with an atom and the confirmation button? Anyway the notif screen will go away so doesn't matter. We should merge
There was a problem hiding this comment.
FTR I got it to work by rewriting it like this:
(defn confirm-code []
(let [auto-focus? (atom true)]
(fn [{:keys [confirm-failure? processing? view-width]}]
(if processing?
(do (reset! auto-focus? false)
[react/view {:style {:justify-content :center
:align-items :center}}
[react/activity-indicator {:size :large
:animating true}]
[react/text {:style {:color colors/gray
:margin-top 8}}
(i18n/label :t/processing)]])
[password-container confirm-failure? view-width @auto-focus?]))))
There was a problem hiding this comment.
I think the problem is due to the fact that when processing? is done password-container is re-rendered
There was a problem hiding this comment.
maybe a cond in intro-step-forward for when account is created that to a navigation reset? because currently the problem is that you can navigate back and you shouldn't be able to
There was a problem hiding this comment.
That's a great hint, tried it and it works, and also fixes the keyboard issue. Thanks @yenda :)
yenda
reviewed
Oct 10, 2019
yenda
reviewed
Oct 10, 2019
| (defn enable-notifications [] | ||
| [vector-icons/icon :main-icons/bell {:container-style {:align-items :center | ||
| :justify-content :center} | ||
| :width 66 :height 64}]) | ||
|
|
||
| (defn bottom-bar [{:keys [step generating-keys? weak-password? encrypt-with-password? | ||
| (defn bottom-bar [{:keys [step weak-password? encrypt-with-password? |
There was a problem hiding this comment.
it was already there so let's not change it but I think that in the future we should avoid having these big step based components
|
@siphiuel I think this is ready for @andmironov and @churik to have a look? |
|
@andmironov @errorists can you please have a look at this again? |
|
@siphiuel tested on IOS 11.4.1(IPhone 7) and Android 8(LG V20):
One small issue that was introduced by fixes in navigation:
|
|
@churik fixed navigating back from Recovery wizard, thanks for spotting this. |
100% of end-end tests have passedPassed tests (47)Click to expand |
Signed-off-by: yenda <eric@status.im>
yenda
force-pushed
the
fix/8551-onboarding-ui-touchups
branch
from
b0ac23c to
5fd269f
Compare
|
Basic recovery looks good on iOS to me! |
|
@andmironov this one is finished, please create a new issue with all flaws, i'll fix it, i still can see screens are broken on small device, and also i can break the app by pressing the back button |
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 #8551
Fixes #8911
Fixes #9054
For some reason
Learn morebottom sheets were erased from code, brought these back. Also added FaceID screen that was originally missing.Copying separate items from the related issue with added notes.
General
Notification icon is cut off at the bottomCould not reproduce
Navigation is too high, should be in header 44px from the topAdded a top margin there
Keycard icon scales on Samsung Galaxy x and becomes unreadableCould not reproduce
There's noticeable padding on both sides of the image carousel when scrolling. The carousel should stretch the entire width of the screen. The dots below the carousel should be switched on/off in the same event - when page has changed. Currently it looks like when on scroll start they switch off and if the page changed, they switch back on.Fixed
Welcome screens
The welcome screen cuts off the images and text, should not be this way (see screenshot below)Fixed
on "Get yourself a key" screen, when tapping on "Generate a key" button, the image moves a few pixels downFixed
If I tap "Generate a key" -> See the list of available account names -> tap "back" -> tap "Generate a key" again -> there's a bug: infinite spinner "Generating a key"Could not reproduce
Choose a key and name screen
Wrong layout (margins around screen title and the list) (see Figma)@andmironov can you please be more specific? I checked the margins but don't understand which ones exactly are off.
Bottom sheet (the one you see after tapping on "Learn more") has wrong margins (see Figma)Fixed. However, there's something wrong with font size/line heights. In Figma, I see that bottom sheet title font-size/line-height are 17/21, while we don't have this combination in our typography namespace. Typography style
title-boldis the closest match (17/23), and this is what's being used here.Wrong margins In list items (see Figma)Fixed. I've noticed that in Figma key-name/storage-type titles have font-size/line-height equal to 15/22, and in our typography the closest match is default-style which is 15/21. React Native's inspector shows line-height to be 22.9 in Android and 18.3 in iOS. Is it due to bold typeface? Also, heights of account entries are different: on iOS it's 64 (like in Figma), on Android it's 67.4. So it's quite difficult to adjust heights in both iOS and Android.
The line that separates the "Next" button is not 100% wide (see Figma)Fixed
Select key storage screen
Slightly wrong layout (see Figma)@andmironov Can you please provide more details?
List items have slightly wrong margins (see Figma)Fixed
Bottom sheet (the one you see after tapping on "Learn more") has wrong margins (see Figma)Fixed
Create password screen
The cursor follows the opening keyboard, overlays the text@andmironov what does this mean? Can you please provide a screenshot? Maybe I just can't reproduce on my device.
The line that separates the "Next" button is not 100% wide (see Figma)Fixed
The typeface of the password is too bigIf I recall correctly, we discussed earlier with @errorists that it should match the title typography. I could have understood it wrong. Can you please clarify?
The margin around the "Next" button is too big (see Figma)Adjusted margins
Enable fingerprint
I have an iPhone X, why am I seeing this?Added FaceID screen. Will check once builds are ready.
The "Sync using mobile data" bottom sheet appears here. Does not make sense, I think we should show it after the setup is complete, maybe after the second launch of the appCouldn't reproduce, will try once builds are ready