Feature/user auth flow #25
Conversation
Sirbuland
force-pushed
the
feature/user_auth_flow
branch
from
7a76163
to
0e728b2
Compare
| this.signoutButtonTarget.addEventListener("ajax:success", (event) => | ||
| this.signoutUserHandler(event) | ||
| ); | ||
| } |
There was a problem hiding this comment.
what's with the guards on these? Are there times when these targets don't exist?
There was a problem hiding this comment.
@hcatlin yeah these are alternative when user logged in loginButtonTarget & hasSignupButtonTarget doesn't exist. And when use is not login signoutButtonTarget doesn't exist.
| createModal(response, target) { | ||
| this.modalLabelTarget.innerHTML = | ||
| target === "login" ? "Sign in to Veue" : "Create Account Veue"; | ||
| document.body.style.overflowY = "hidden"; |
There was a problem hiding this comment.
Why do we have to change the overflow on the body when this shows up? I'd at least document this!
There was a problem hiding this comment.
@hcatlin we change overflowY to stop background page scroll otherwise it will scroll while showing model.
| - else | ||
| %ul | ||
| %li | ||
| = link_to "Login", "javascript:void(0)", class: "list-item", data: { target: "authentication.loginButton" } |
There was a problem hiding this comment.
OH! I think you really want data-actions here! data: { action: "click->authentication.login" }
There was a problem hiding this comment.
This explains why you have that whole wiring up event listeners if statements... none of that is needed!
There was a problem hiding this comment.
Hmm ok. I'll change it and update PR shortly.
Sirbuland
force-pushed
the
feature/user_auth_flow
branch
from
5d189a5
to
0e728b2
Compare
Sirbuland
force-pushed
the
feature/user_auth_flow
branch
from
0e728b2
to
e70133f
Compare
| @@ -4,6 +4,8 @@ class ApplicationController < ActionController::Base | |||
| include HttpAuthConcern | |||
| include DeviseHelperConcern | |||
|
|
|||
| protect_from_forgery unless: -> { request.format.json? } | |||
There was a problem hiding this comment.
Can you add a TODO here to remove this? Big security hole!
This PR is implementing following functionalities: