-
Notifications
You must be signed in to change notification settings - Fork 984
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
Change password inside new settings #19474
Conversation
Jenkins BuildsClick to see older builds (86)
|
src/status_im/contexts/profile/settings/screens/password/change_password/new_password_form.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/new_password_form.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/old_password_form.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/view.cljs
Outdated
Show resolved
Hide resolved
e4da076
to
76952ec
Compare
@cammellos addressed your comments in the last commit |
76952ec
to
a5c1f3e
Compare
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.
Nice stuff 🙌
Left some small comments, I'll doing another pass soon 🙏
src/status_im/contexts/profile/settings/screens/password/change_password/new_password_form.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/effects.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/events.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/loading.cljs
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/style.cljs
Outdated
Show resolved
Hide resolved
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.
Hey @clauxx, I wasn't added as a reviewer, but I felt like sharing a few comments 😈
src/status_im/contexts/profile/settings/screens/password/change_password/effects.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/new_password_form.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/new_password_form.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/effects.cljs
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/loading.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/profile/settings/screens/password/change_password/new_password_form.cljs
Show resolved
Hide resolved
a14e999
to
078cf53
Compare
@clauxx is this ready to be tested again? |
@cammellos @mariia-skrypnyk yes ready for testing |
Hi @clauxx ! As we keep our Test mode banner we still need to figure out how to deal with This issue is not visible on my screenshots (security restriction) but I can show you it on video: We have two places on
video_2024-04-30_12-02-28.mp4video_2024-04-30_12-02-32.mp4Android doesn't contain such issue as there is a scroll there and video_2024-04-30_12-15-57.mp4ISSUE 7: Additional screens are blinking after logout Steps
Actual result: video_2024-04-30_12-16-02.mp4Expected result: no such blinking screens |
hi @clauxx this should fix issue 6.
|
:on-press navigate-back}] | ||
[rn/keyboard-avoiding-view | ||
{:style {:flex 1} | ||
:keyboardVerticalOffset (- (safe-area/get-bottom))} |
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.
here, in ios also subtract alert-banners-top-margin
, that should fix the issue.
@mariia-skrypnyk issue 7 seems to be low-priority, would it be fine to address separately and merge it if that's the only issue left? |
Sure 🙌 |
@mariia-skrypnyk @Parveshdhull @cammellos @J-Son89 I pushed a fix, but it's a bit annoying that we have to add such changes due to the alert banner alone. Also, among other issues, when building new features where the test-mode banner doesn't matter (e.g. here), we have to always keep it in mind and check if the UI is messed up since the banner is also disabled on debug builds by default. I remember there was a thread about the banner topic, but not sure if there was any resolution and hopefully we can get back to it soon and figure out a way that is less ... intrusive. |
Hi @clauxx ! Thanks for you effort in this PR!
IMG_9047.MP4@clauxx I am going to check PR on iPhone SE and see no blockers to merge it after. |
@mariia-skrypnyk Yeah might be better to handle this as a follow-up. Thank you :) |
81% of end-end tests have passed
Failed tests (8)Click to expandClass TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (42)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
|
Hi @clauxx ! Checked on iPhone SE, found minor issue, will add to follow up. Failed tests are not related, please merge! |
fixes #19111
Summary
change password
screens according to the current designs (which might change as they're not final)status-im.contexts.onboarding.create-password.view
nsenable biometrics
steppassword-tips
quo componentReview notes
Testing notes
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready