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 post functionality to singup #86

Merged
merged 4 commits into from
Jul 5, 2024
Merged

Conversation

Pybite
Copy link
Collaborator

@Pybite Pybite commented Jul 5, 2024

resolved issue #82

const navigate = useNavigate()

const onChange = e => {
setInputs({ ...inputs, [e.target.name]: e.target.value })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
setInputs({ ...inputs, [e.target.name]: e.target.value })
setInputs({ ...inputs, [e.target.name]: e.target.value })

Comment on lines 25 to 41
e.preventDefault();
const body = { name, email, password, dateOfBirth, biography, notificationPreference, agreeToTerms, subscribeToNewsletter };
try {
const r = await fetch('http://localhost:3001/users', {
method: 'POST',
headers: { 'Content-type': 'application/json' },
body: JSON.stringify(body),
});
const response = await r.json();
if(response){
navigate('/signin');
}

} catch (error) {
console.error(error.message);

}
Copy link
Member

@nbkhope nbkhope Jul 5, 2024

Choose a reason for hiding this comment

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

Whenever you open curly brace, indentation increases
(for readability)

Suggested change
e.preventDefault();
const body = { name, email, password, dateOfBirth, biography, notificationPreference, agreeToTerms, subscribeToNewsletter };
try {
const r = await fetch('http://localhost:3001/users', {
method: 'POST',
headers: { 'Content-type': 'application/json' },
body: JSON.stringify(body),
});
const response = await r.json();
if(response){
navigate('/signin');
}
} catch (error) {
console.error(error.message);
}
e.preventDefault();
const body = { name, email, password, dateOfBirth, biography, notificationPreference, agreeToTerms, subscribeToNewsletter };
try {
const r = await fetch('http://localhost:3001/users', {
method: 'POST',
headers: { 'Content-type': 'application/json' },
body: JSON.stringify(body),
});
const response = await r.json();
if(response){
navigate('/signin');
}
} catch (error) {
console.error(error.message);
}

Comment on lines 33 to 36
const response = await r.json();
if(response){
navigate('/signin');
}
Copy link
Member

@nbkhope nbkhope Jul 5, 2024

Choose a reason for hiding this comment

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

When you call await fetch inside a try block, if for some reason you can't reach the server, an error will be raised and execution will go to the catch block. (You dont have to check anything)

Now, if you get a response from the server, it could be two:

  1. All good (r.ok is true)

  2. Server didnt like your request for some reason and rejected it with an error (r.ok is false)

So you can do like this:

Suggested change
const response = await r.json();
if(response){
navigate('/signin');
}
if (r.ok === false) {
// raising an error here moves execution to catch block.
throw new Error('Server rejected the request for some reason');
}
// else we keep going (r.ok is true):
// actually we dont even need responseBody or to parse the body if it was OK
// so you could just remove this line
const responseBody = await r.json();
navigate('/signin');

(I might have missed some syntax in the code suggestion above)

<label for="sign-up-form-name">Name</label>
<input type="text" id="sign-up-form-name" name="name" required/>
<label htmlFor="sign-up-form-name">Name</label>
<input type="text" id="sign-up-form-name" name="name" value={name} onChange={e => onChange(e)} required/>
Copy link
Member

@nbkhope nbkhope Jul 5, 2024

Choose a reason for hiding this comment

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

you can just pass the function definition as is (same applies to other similar lines)

Suggested change
<input type="text" id="sign-up-form-name" name="name" value={name} onChange={e => onChange(e)} required/>
<input type="text" id="sign-up-form-name" name="name" value={name} onChange={onChange} required/>

</div>
<div className="form-group">
<div>
<label><input type="checkbox" name="agreeToTerms" value="true"/> I agree to the terms and conditions</label>
<label><input type="checkbox" name="agreeToTerms" value='true' onChange={e =>onChange(e)} /> I agree to the terms and conditions</label>
Copy link
Member

@nbkhope nbkhope Jul 5, 2024

Choose a reason for hiding this comment

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

Did you see if the checkbox actually checks if you click? I think here you gotta look at checked instead of value.

checked will be either true or false.

It's probably gonna check visually but you are changing from the value which is not what you want. You could try console.log(inputs) before return and check/uncheck to see if the value changes. (Or you could use debugger in dev tools). My suspicion is the value in memory actually doesnt go to false if unchecked after checked (despite you visually seeing it unchecked).

Suggested change
<label><input type="checkbox" name="agreeToTerms" value='true' onChange={e =>onChange(e)} /> I agree to the terms and conditions</label>
<label><input type="checkbox" name="agreeToTerms" value='true' onChange={e => setInputs({ ...inputs, [event.target.name]: event.target.checked })} checked={agreeToTerms} /> I agree to the terms and conditions</label>

Copy link
Member

@nbkhope nbkhope Jul 5, 2024

Choose a reason for hiding this comment

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

My suspicion confirmed:
image

@Pybite Pybite requested a review from nbkhope July 5, 2024 17:21
@nbkhope nbkhope merged commit e9b1f3d into nbktechworld:master Jul 5, 2024
1 check passed
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.

None yet

2 participants