Improve login screen when only OmniAuth providers are enabled #7473
Conversation
|
I've prepared a stage. Click to open. |
|
👍 |
|
I also noticed there's an extra HR. Will update the commit in a minute. Sadly I think there's some unavoidable spaghetti logic here :| |
|
Before After |
|
@randx Can you take a look? |
|
|
dblessing
added
Awaiting developer action/feedback
and removed
Awaiting feedback
labels
|
@sodabrew Please see the comments about |
|
Thanks for the reviews! I'll try to take a look at updating the PR this week. |
sodabrew
force-pushed
the
patch-1
branch
2 times, most recently
from
94b0a89
to
94cf618
Compare
|
Ping, developer action was taken. |
|
Thanks @sodabrew. I'll take a look shortly. |
| %div | ||
| No authentication methods configured. | ||
|
|
||
| = render 'devise/sessions/oauth_providers' if Gitlab.config.omniauth.enabled && devise_mapping.omniauthable? | ||
| - if Gitlab.config.omniauth.enabled && devise_mapping.omniauthable? |
There was a problem hiding this comment.
No, because you can have Login, LDAP, both, or neither, and then you can also have Omniauth providers.
|
@sodabrew UI is exactly the same as the original screenshots? See my one comment on |
| @@ -20,11 +20,15 @@ | |||
|
|
|||
| - elsif gitlab_config.signin_enabled | |||
| = render 'devise/sessions/new_base' | |||
| - else | |||
|
|
|||
| - elsif !Gitlab.config.omniauth.enabled || !devise_mapping.omniauthable? | |||
There was a problem hiding this comment.
I propose that we move this block to the end and use else instead of elsif. If everything above it fails then we will want to display this without any further conditions.
|
This looks much better than the |
|
@randx This looks good. |
|
@dblessing @randx Anything I can do to help move this PR forward? Should I put up new screenshots, since the login page has changed a bit since August? |
|
I will take a look at 7.8. Sorry we are busy with release now |
|
@sodabrew In this next few days please rebase so this is mergeable when Dmitriy gets a chance to come back to this for 7.8. |
|
Will do, thx. |
|
We can only accept a merge request if all the tests are green. I've just restarted the build. When the tests are still not passing after this restart and you're sure that is does not have anything to do with your code changes, please rebase with master to see if that solves the issue. |
|
The failure is unrelated. Tell me when your master branch isn't broken and I'll rebase. |
Avoids an empty Sign in box when signup_enabled? is false, and avoids showing "No authentication methods configured" unless there really are none. OmniAuth signin gets its own file for consistency with signin and signup and LDAP.
|
The signin files changed a lot, rebased to match up with new layout. Here's the problem with current master. The solution is to not draw the first box if there are omniauth providers defined. |
jvanbaarsen
added
Ready for Merge
and removed
Awaiting developer action/feedback
labels
|
Looks good. Thank you |
Improve login screen when only OmniAuth providers are enabled
Hide the text "No authentication methods configured." when Signin and LDAP are disabled but OmniAuth is enabled.
Hide the link to "Did not receive confirmation email?" when Signup is disabled.