Nudge people to the organisation login if OIDC enabled. #4349
Conversation
|
@stevenbal could you prettify this code and port it? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4349 +/- ##
=======================================
Coverage 96.26% 96.26%
=======================================
Files 731 731
Lines 23786 23787 +1
Branches 2803 2803
=======================================
+ Hits 22898 22899 +1
Misses 617 617
Partials 271 271 ☔ View full report in Codecov by Sentry. |
| @@ -1,11 +1,14 @@ | |||
| {% extends "maykin_2fa/login.html" %} | |||
There was a problem hiding this comment.
@joeribekker I'm not sure if I can port this to mozilla-django-oidc-db easily, since our template extends the login template from maykin_2fa currently. I could maybe create a snippet for the extra login options in mozilla-django-oidc-db that is included here?
There was a problem hiding this comment.
Maybe port it to default project, as that is our glue for all projects
| /** | ||
| * mozilla-django-oidc-db | ||
| * hide the regular admin login form by default if OIDC is enabled | ||
| */ | ||
| #login-form:has(~ .admin-login-oidc) { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
I think I'd make these selectors more component-oriented with the proposed class names to follow BEM methodology.
.admin-login-option {
@include bem.modifier('username') {
~ .admin-login-option--oidc {
display: none;
}
}
}There was a problem hiding this comment.
But the rule at line 122 is meant to set display: none; on the #login-form itself, whereas your suggestion hides the OIDC option instead (if I understand it correctly)?
There was a problem hiding this comment.
I didn't try out the actual CSS, so yeah, probably should throw in a :has(...) in there! My point was more about using the BEM methodology :)
| &--disabled { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
can you please use the mixins like in the sample I gave? These exist because the resulting selector now is .admin-login-option--disabled, and with the mixin it is .admin-login-option.admin-login-option--disabled.
A classname with modifier may not exist without the block/element itself, these sass mixins help enforce this.
There was a problem hiding this comment.
I added it for .admin-login-option--disabled, though I'm not sure if this works for the #login-form block, because it's an id instead of a class 🤔
There was a problem hiding this comment.
yeah it doesn't work with IDs, since the whole idea of BEM is class-names :) for those you can spell them out manually entirely. ID's are an anti-pattern anyway for styling, I dislike that Django uses them so much in their CSS :(
* use `onLoaded` instead to add admin form toggle functionality * use BEM syntax for admin login options * fix admin-login-divider styling and use CSS var instead of hardcoded colors
stevenbal
force-pushed
the
feature/better-login-ux
branch
from
8c5539f to
928484d
Compare
Closes #4347
Changes
Hides the form when OIDC is disabled. Note that this is not the best code, but it shows the intent of what I want. It has 2 aspects: 1) nicer and better visible -or-, 2) less nudge towards form login
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh.sh./bin/compilemessages_js.shCommit hygiene