-
Notifications
You must be signed in to change notification settings - Fork 11
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
Improve log in page button handling #582
Conversation
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.
Please, check comment below.
src/login/LoginMainPage.tsx
Outdated
@@ -324,6 +329,9 @@ const LoginMainPage = () => { | |||
isValidPassword={isValidPassword} | |||
onLoginButtonClick={onLoginButtonClick} | |||
loginButtonLabel="Log in" | |||
isLoginButtonDisabled={ |
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.
Technically, the login button is still needed to login via Kerberos (it doesn't require to have any username or password filled, and in the case of authentication via Certificate it requires just the username). So I'm not sure if this specific lines are needed:
isLoginButtonDisabled={
authenticating || username === "" || 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.
Ok that's fine, but the fields are marked as required in the UI. Should this be changed as well?
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.
I'm sorry, I'm not sure what do you mean by 'required'.
Just to clarify, the username and password fields are initialized as empty and those are set and handled by the LoginForm
component, so we are just passing the variables to it. As far as I know, there is no requirement that those values should be empty.
const loginForm = (
<LoginForm
showHelperText={showHelperText}
helperText={errorMessage}
helperTextIcon={<ExclamationCircleIcon />}
usernameLabel="Username"
usernameValue={username} // <--- Here
onChangeUsername={handleUsernameChange}
isValidUsername={isValidUsername}
passwordLabel="Password"
passwordValue={password} // <--- Here
isShowPasswordEnabled
onChangePassword={handlePasswordChange}
isValidPassword={isValidPassword}
onLoginButtonClick={onLoginButtonClick}
loginButtonLabel="Log in"
isLoginButtonDisabled={
authenticating || username === "" || 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.
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.
Oooh I see, thanks for clarifying. Yes, it seems to be something hardcoded in the PatternFly component. What I'm not sure if this is something that we can change at all. I will ask the PF team via Slack just in case.
Disable the log in button while "logging in" is in progress, or if the username and password are empty Fixes: freeipa#581 Signed-off-by: Mark Reynolds <[email protected]>
490af62
to
9d17a32
Compare
@carma12 I made the requested changes, please review... |
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.
I have also contacted the PatternFly team via Slack to ask them more information about the required username and password fields. It seems that they are open to provide a new parameter to the LoginPage
component to allow disable those fields shown as required. Will keep you posted about it.
Disable the log in button while "logging in" is in progress, or if the username and password are empty
Fixes: #581