-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Provide error feedback without actually submitting #1873
Conversation
d74effa
to
3653042
Compare
app/components/Views/Login/index.js
Outdated
@@ -332,7 +334,7 @@ class Login extends PureComponent { | |||
|
|||
render = () => { | |||
const { password } = this.state; | |||
const disabled = !passwordRequirementsMet(password); | |||
const locked = !passwordRequirementsMet(password); |
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.
shouldn't we have this on onLogin
? feels like we're doing calculations on render that could be moved to do it just once
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.
actually that's a good point, now that we're not doing anything in the render (disabling the button), we could just move it to onLogin
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.
QA Passed 👍
My earlier idea of keeping the login button locked is just going to be confusing to users. We should provide feedback and keep the button enabled. But! We're still not attempting to Unlock for real, which I think is ultimately what we want here.
8f0a08c
to
5f17dcb
Compare
My earlier idea of keeping the login button locked is just going to be confusing to users. We should provide feedback and keep the button enabled. But! We're still not attempting to Unlock for real, which I think is ultimately what we want here.
My earlier idea of keeping the login button disabled is just going to be confusing to users (I find it confusing myself now that I've been using it). We should provide feedback and keep the button enabled. But! We're still not attempting to Unlock for real (until requirements are met), which I think is ultimately what we want here.
Checklist
Issue
Resolves #???