-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added AuthComponent #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one change and we are ready to merge ;)
className = 'a6y-react-auth', | ||
}: IAuthProps): JSX.Element => { | ||
useEffect(() => { | ||
if (config && config.currentForm) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we spoke, we don't read it from the config. We can use useEffect
to find a specific query param, ie: f=sign-in
. And then if any is set, we can set it in the currentForm
.
However, I'm wondering if it could be done inside of useState
(I mean to read the query param). It's because that if we use it useEffect, the page (maybe) will be rendered twice. First time, before useEffect will run, and then after the useEffect sets the currentForm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, you can just remove useEffect
. We can add support for query params later.
@@ -30,7 +30,7 @@ const SignInContainer = ({ className }: ISignInContainerProps): JSX.Element => { | |||
} | |||
} | |||
return ( | |||
<div className={className ? className : 'a6y-react-auth__sign-in'}> | |||
<div className={className ? className : 'a6y-react-auth__sign-in-cnt'}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to change here, just sharing my thoughts that maybe we should figure out some better naming convention for CSS classes. I believe that -cnt
means the "container" here, but is it named that way because it is in SignInContainer
file or because this div
acts as a container/wrapper.
Maybe in the future, we will try to standardize it a bit.
No description provided.