Skip to content
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 login pages, and register/login functionality #15

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jhcoll
Copy link
Owner

@jhcoll jhcoll commented Dec 2, 2021

No description provided.

@jhcoll jhcoll force-pushed the feature_login_page branch from 60b0be1 to fca2e11 Compare December 2, 2021 14:17
Copy link
Collaborator

@jluxford-scottlogic jluxford-scottlogic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice set of changes, looks like logging in works well

const [showError, setShowError] = useState(false);
const [errorText, setErrorText] = useState("");
const [idToken, setIdToken] = useState();
const [userIdToken, setUserIdToken] = useState();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between userIdToken and idToken?

const navigate = useNavigate();
const emailRef = useRef();
const passRef = useRef();
let fireResp = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably fine but it isn't a very reacty thing to do. A quick improvement would be to move it inside the function createAccount(account) because there at least the side effects are managed. In general though with good react there should be no side-effects inside a component which means that all variables should be consts at the top level or hooks if state is required. The better option might be to attempt to embed the boolean inside the data of the .then though this might not necessarily be possible

<div className={classes.content}>
<div className={classes.input}>
<input required placeholder="Email" ref={emailRef}></input>
<input required placeholder="Password" ref={passRef}></input>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice if the password is hidden behind *s or dots when displayed, might be manageable using hooks and states or a configure option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants