DEV: Convert login modal to component-based API
#23093
Conversation
| @flashTypeChanged={{this.flashTypeChanged}} | ||
| @securityKeyCredentialChanged={{this.securityKeyCredentialChanged}} | ||
| /> | ||
| <Modal::Login::Footer |
There was a problem hiding this comment.
It is not ideal to render the <div class="modal-footer"> within the body of the modal, but I am limited in what I can do with the current design of the login modal. I don't think we are necessarily limited by the current structure of the modal (body / footer) we are limited by the structure of the css / html implemented to make it look the way it does.
This would be worth raising with designers once these changes are through
janzenisaac
added
the
js-modernization
| // I don't think this lives in the right place, this feels like it should live in some kind of auth service / controller | ||
| // and then be called in routes/application.js | ||
| // we render the login modal but then immediately redirect to the external auth service | ||
| // I think we can skip the login modal and just redirect to the external auth service | ||
| @action | ||
| async externalLogin(loginMethod, { signup = false } = {}) { | ||
| if (this.loginDisabled) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| this.loggingIn = true; | ||
| await loginMethod.doLogin({ signup }); | ||
| this.args.closeModal(); | ||
| } catch { | ||
| this.loggingIn = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this lives in the right place, this feels like it should live in some kind of auth service / controller and then be called in routes/application.js. We render the login modal but then immediately redirect to the external auth service 🤔 . I think we can skip the login modal and just redirect to the external auth service...
Maybe a refactor for a different PR
janzenisaac
force-pushed
the
upgrade-login-modal
branch
from
8a103a9
to
1b0e8c0
Compare
There was a problem hiding this comment.
This block of tests is reliant on the legacy login modal, so it has completely fell apart. I know it feels bad to remove an entire test file but in my mind we are left with only two options:
- Spend time rewriting these tests to either
- rely on one of the few remaining legacy modals
- use some generic legacy modal controller (🤷)
so that we can have a safety net on our new modal API for a bit longer.
- Rip off the bandaid and save some time by deleting the tests
By deleting the file, you can see I am obviously in favor of option 2. We are a couple weeks away from deleting them either way, and I feel like we have lived with the new modal API changes long enough we can now safely delete. What do you think @davidtaylorhq
| constructor() { | ||
| super(...arguments); | ||
| if (this.args.model?.isExternalLogin) { | ||
| this.externalLogin(this.args.model.externalLoginMethod, { | ||
| signup: this.args.model.signup, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Once this PR lands I would love to come back in and refactor this block. We render the Login modal in multiple places to then immediately redirect to an external auth service. I would like to skip the intermediate LoginModal step.
janzenisaac
force-pushed
the
upgrade-login-modal
branch
from
0b80aa6
to
1c3e395
Compare
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/oauth2-custom-redirects-plugin/94323/2 |
janzenisaac
force-pushed
the
upgrade-login-modal
branch
from
1c3e395
to
d06f10b
Compare
janzenisaac
force-pushed
the
upgrade-login-modal
branch
from
b3786fa
to
2ade95a
Compare
janzenisaac
force-pushed
the
upgrade-login-modal
branch
from
7005433
to
aa27102
Compare
| @@ -52,27 +54,6 @@ export default Controller.extend( | |||
| return (hasAuthOptions || canCreateLocal) && !skipConfirmation; | |||
| }, | |||
|
|
|||
| resetForm() { | |||
| // We wrap the fields in a structure so we can assign a value | |||
| this.setProperties({ | |||
There was a problem hiding this comment.
How come we're making this change in the create-account modal? This one is still a legacy controller, so I would've thought that we still need the manual reset?
This PR relies upon #23093 - move login modal tests to `..../modal/login/....`
Desktop
Before
After
Mobile
Before
After
Changes Made
Before
After