-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add error to login input #14627
Add error to login input #14627
Conversation
@aimane-chnaif @youssef-lr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@luacmartins any reason why only form validation errors should show a red border? |
The API doesn't return the specific input the error is related to, so right now server errors are shown as a form level error instead of individual input. They are also not translated like other input errors. This is consistent with other forms in the App, e.g. VBA forms. |
Haha I see. I guess we can live with just a couple of forms that aren't using |
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.
LGTM and tested well!
reviewing now |
I agree LoginForm and PasswordForm won't be converted to Form for now but just imitated. The inconsistent behavior is that:
@luacmartins Please correct me if I am wrong. If correct, are we fine with this for now? |
@aimane-chnaif I think those make sense for the login forms and is still the same behavior we have on staging/prod. |
Maybe Magic code should be added in test case as well since passwordless flow is now live in beta? |
Sure, let me look into that |
Updated! |
Reviewer Checklist
Screenshots/VideosWeb(magic code) web.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.mp4Desktopdesktop.moviOS(no 2fa) ios.mp4Androidandroid.mov |
@youssef-lr could you please re-review/approve this? |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.2.66-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.2.66-0 🚀
|
I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check. We've also updated the item in the checklist with this PR to avoid this issue in the future. |
Details
Fixes how we display errors on the login form to align with other forms within the App, i.e.:
Fixed Issues
$ #14622
Tests
Pre-requisite: an account with 2FA enabled
Email
Please enter an email or phone number
error and the input border is reda
Continue
The email entered is invalid. Please fix the format and try again.
error and the input border is red1234
Continue
I couldn't validate the phone number, please try again with the country code (e.g. +15005550006).
error and that the input border is GREEN (this is a server error)Continue
Password
Sign in
Please enter your password
error and the input border is reda
Sign in
Incorrect password. Please try again.
error and the input border is redPassword1
Sign in
Incorrect login or password. Please try again or reset your password.
error and that the input border is GREEN (this is a server error)Sign in
2FA
Sign in
Please enter your two factor code
error and the input border is red1
Sign in
Incorrect two factor authentication code. Please try again.
error and the input border is red123456
Incorrect Two Factor Authentication code. Please try again.
error and that the input border is GREEN (this is a server error)Sign in
Offline tests
Email
You appear to be offline.
message below the buttonContinue
button is disabledContinue
Password & 2FA
You appear to be offline.
message below the buttonSign in
button is disabledQA Steps
Same as test and offline steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov