Improve Login Selector UX #64142
Improve Login Selector UX #64142
Conversation
azasypkin
added
blocker
blocked
Team:Security
|
Pinging @elastic/kibana-security (Team:Security) |
|
Hey @snide! Would you mind clarifying few more things regarding Login Selector improvements discussed in #61144 for me? Let me try to describe the possible states and questions I have. It's a very rough draft, I only use standard EUI components without any additional styling yet, just to have something to reason about.
Thanks! |
|
hey @azasypkin Rather than do a back and forth on the individual screens, it might be easier to do a small writeup on how I can get my dev setup to show the options as shown. I don't mind jumping in the code (or having one of our other coding designers jump in) to help out and clean up the interactions. If that's too hard, possibly we can just fake some states in the draft PR so we can fill in the bits. I'll take a scan at what's in here now and see what I can manage. |
Yep, makes sense to me. I've updated issue description with the Kibana config that one can use with this PR. Technically you don't need need to perform actual SSO login to test the flows so there is no need to re-configure ES for that, but if you'd like to be able to log in and use Kibana with SAML I can share required configuration over slack (there is some sensitive IdP info we can't share here). I'm happy to Zoom/Slack to help with the setup or answer any questions if needed. Thanks! |
|
Excellent. I've give it a shot, and if I can't figure it out today. I'll set something up tomorrow early my time and we can pair. |
|
@azasypkin Hope its ok. I pushed directly to your PR. I made the following changes:
For wording of the "Kibana" login, I'm gonna check with some others tomorrow and update. The best I could come up with was being very on the nose about it being "Elasticsearch".
In any case. I probably need another commit in here to clean things up some more. If you give me a way to render the "get help" stuff I can work on that part as well. |
Looks awesome, thanks!
Yeah, it's much closer to the truth than
Sure, for now it's xpack.security.loginHelp: "**You are accessing a system with U.S. government information**" |
|
OK @azasypkin I'm done with the design work. I'll assume you're good to go, but feel to ping me on review when you finish up. |
Yeah, I can go ahead and prepare this PR for review. Thanks a lot for the help, @snide!
Did you have a chance to check this? Or we're moving forward with Also |
azasypkin
force-pushed
the
issue-61144-login-improvements
branch
from
29751e4
to
903f031
Compare
|
@azasypkin and I chatted over Slack. Setting "Log in with Elasticsearch" as the default. Need help will be empty by default (so not used). Cloud is strongly recommended to use it for guidance and a way to link to support directly. |
azasypkin
force-pushed
the
issue-61144-login-improvements
branch
from
903f031
to
f38790b
Compare
azasypkin
force-pushed
the
issue-61144-login-improvements
branch
from
f38790b
to
6daeb5d
Compare
|
ACK: will review on Monday |
There was a problem hiding this comment.
This is working great! Just a couple nits/questions.
- Let's also make sure to update
kibana-dockerto include the new settings
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <EuiPanel> | ||
| <div className="lgnAssistanceMessage"> |
There was a problem hiding this comment.
nit: Other classes within the security plugin start with sec. We should probably maintain this "namespace"
There was a problem hiding this comment.
Yeah, I'll switch to sec or secLogin (will see) 👍
There was a problem hiding this comment.
That's fine to change. @azasypkin I'll leave these to you
| ? this.onPageModeChange(PageMode.Form) | ||
| : this.loginWithSelector(provider.type, provider.name) | ||
| } | ||
| className={`lgnCard ${ |
There was a problem hiding this comment.
nit class name should start with sec
| <EuiIcon size="xl" type={provider.icon ? provider.icon : 'empty'} /> | ||
| </EuiFlexItem> | ||
| <EuiFlexItem> | ||
| <EuiTitle size="xs" className="lgnCard__title"> |
There was a problem hiding this comment.
nit class name should start with sec
| )} | ||
| </p> | ||
| </EuiTitle> | ||
| {provider.hint ? <p className="lgnCard__hint">{provider.hint}</p> : null} |
There was a problem hiding this comment.
nit class name should start with sec
| <EuiText size="xs" className="eui-textCenter"> | ||
| <EuiLink | ||
| data-test-subj="loginBackToLoginLink" | ||
| style={{ fontWeight: 'bold' }} |
There was a problem hiding this comment.
@snide this link to return back to the login flow is bold, but the link to get to this screen is not bolded. Is that intentional?
There was a problem hiding this comment.
I though it was intentional, but thanks for double checking 🙂
There was a problem hiding this comment.
The font-weight style tag should be removed.
| if (this.validator.validateForLogin(username, password).isInvalid) { | ||
| // Since validation is enabled now, we should ask React to re-render form and display | ||
| // validation error messages if any. | ||
| return this.forceUpdate(); |
There was a problem hiding this comment.
We need a new pattern for this across the security screens...we abuse state in other components to achieve this, but your change to use forceUpdate is at least more explicit. You can ignore this comment, I'm just thinking out loud for the future
…x for the styles, do not use bold fond for the back link.
|
@legrego it's ready for another pass, thanks! |
Co-authored-by: Dave Snider <dave.snider@gmail.com>
|
7.x/7.8.0: 75e2b8c |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
In this PR we're going to cover Login Selector UX improvements that have been proposed in #61144 and https://www.figma.com/proto/1yRyEEPb8NMcPOj2cbybQB/Kibana-SSO-login?node-id=1%3A126&scaling=min-zoom
UI/UX
Config
Blocked: by Design Team feedback✔️Fixes: #61144
"Release Note: Login Selector UI was refined and can now offer much more customization options."