-
Notifications
You must be signed in to change notification settings - Fork 8
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
Incorporated axios + set up proxy #35
Conversation
@stygian-96 @Tlazypanda kindly review the changes made :)) |
@Tlazypanda please review. |
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.
@uglyprincess Good job! 🎉 💯 Have left some comments please address them and we are good to go!!
// This function will be called upon clicking the button and will send the login info to the back end. | ||
|
||
function sendInfo(){ | ||
axios.post(`http://localhost:5000/login`, querystring.stringify({username: info.username, password: info.password}), { |
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.
Can we lose the localhost part here in this url?
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.
Will be resolved in latest commit!
|
||
const handleChange = (prop) => (event) => { | ||
updateInfo({...info, [prop]: event.target.value}); | ||
console.log(info); |
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.
Log statements too if testing is done
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.
Sure ma'am, I'll comment them out.
className={classes.inputText} | ||
label="Password" | ||
value={info.password} | ||
onChange={handleChange('password')} |
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.
Are validations added for password and cpassword
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.
Validations for password will be added in the next commit.
.vscode/settings.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ |
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.
Some files here are not required remove them from commit
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.
Sure, will do!
@Tlazypanda all the changes requested have been made. Please review! :)) |
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.
@uglyprincess still some files can be removed from commit like log ...once done merge this rest all looks good 👍
Done ma'am! I believe those were the last of the useless files. |
Pull Request
Proposed Changes
Types of changes made
Additional Info
Viability of Code
Describe the tests that you ran to verify the changes made. Mention the tests for easy reproducibility.
Tested the code on my local server.
Mention(if any) specific Test Configurations used:
Screenshots or GIFs (if any)
Attach screenshots of the changes made for easy reference.
Checklist: