-
Notifications
You must be signed in to change notification settings - Fork 163
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
Log in and Signup pages added #142
Conversation
hey @praveenscience I have added Login and Signup Pages. |
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.
Excellent, but can we please please have the code formatted with Prettier & ES Lint? 😁 We're using this config: My Visual Studio Code Settings
yeah sure @praveenscience |
src/App.js
Outdated
|
||
export default function App() { | ||
return ( | ||
<> |
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.
No need of this. :)
src/components/Header/Header.js
Outdated
@@ -15,6 +16,9 @@ const Header = ({ ChangePage, CurrentPage, Resume }) => { | |||
/> | |||
))} | |||
</ul> | |||
<Link to="/login" className="btn btn-primary btn-sm col-1"> | |||
Login |
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.
Prettier 2 space indentation, please?
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.
Sorry I didn't git this one @praveenscience
src/pages/LoginPage/index.js
Outdated
@@ -0,0 +1,41 @@ | |||
import React from "react"; | |||
import "./index.css"; |
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.
Convert to SCSS, and add it there.
@praveenscience |
There are still couple of changes, I'll take care of them... Will do a check and merge this, alright? |
Yeah fine @praveenscience |
@Siddhesh777 yea, go for it. |
hey @praveenscience |
hey @praveenscience and @code-reaper08 |
Yea, I am giving priorities to this before merging any.... |
@Siddhesh777 can you please add validations to the forms if possible? |
hey @AbhipsaGuru1012 |
Okay, you can take that up in the authentication issue that you have created. |
hey @AbhipsaGuru1012 |
You can work on form validation in this PR itself to make it a complete thing, since form validation is a pretty small piece of work. What you say @praveenscience @AbhipsaGuru1012 ? |
hey @AbhipsaGuru1012 @code-reaper08 |
@Siddhesh777 please add the validations in the Sign-Up page as well. Everything else looks fine! |
hey @praveenscience @AbhipsaGuru1012 @code-reaper08 |
src/pages/SignupPage/index.js
Outdated
const [email,setEmail] = React.useState(""); | ||
const [password,setPassword] = React.useState(""); | ||
const [confirmPassword,setConfirmPassword] = React.useState(""); | ||
const [emailError, setEmailError] = React.useState(""); | ||
const [passwordError, setPasswordError] = React.useState(""); | ||
const [confirmPasswordError, setConfirmPasswordError] = React.useState(""); | ||
|
||
function handleSubmit() { | ||
if (email === "") { | ||
setEmailError("Required"); | ||
} else { | ||
setEmailError(""); | ||
} | ||
if (password === "") { | ||
setPasswordError("No password provided"); | ||
} | ||
else { | ||
setPasswordError(""); | ||
} | ||
if(confirmPassword === "") | ||
{ | ||
setConfirmPasswordError("No confirm password provided"); | ||
} | ||
else | ||
{ | ||
setConfirmPasswordError(""); | ||
} | ||
} |
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 have one form? And where's the prettier formatting?
src/pages/SignupPage/index.js
Outdated
className="inputEmail" | ||
type="email" | ||
placeholder="Email" | ||
autocomplete="off" | ||
onChange={(e)=> setEmail(e.target.value)} | ||
required | ||
/> | ||
<div className="alert">{emailError}</div> | ||
</div> | ||
<div> | ||
<input | ||
className="input" | ||
className="inputPassword" | ||
type="password" | ||
placeholder="Password" | ||
autocomplete="off" | ||
onChange={(e)=>setPassword(e.target.value)} | ||
required | ||
/> | ||
<div className="alert">{passwordError}</div> | ||
</div> | ||
<div> | ||
<input | ||
className="input" | ||
className="inputPassword" | ||
type="password" | ||
placeholder="Confirm Password" | ||
autocomplete="off" | ||
onChange ={(e)=> setConfirmPassword(e.target.value)} | ||
required | ||
/> | ||
<div className="alert">{confirmPasswordError}</div> | ||
</div> | ||
<div className="Submit-form"> | ||
<div> | ||
<input className="inputSubmit" type="submit" value="Signup" /> | ||
<input className="inputSubmit" type="submit" value="Signup" onClick={handleSubmit}/> | ||
</div> | ||
<div> |
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.
Don't use classless divs and also can we automate this instead of repeating?
src/pages/SignupPage/index.scss
Outdated
.alert { | ||
color: red; | ||
text-align: left; | ||
margin: 0px; | ||
margin-left: 12rem; | ||
padding: 0px; | ||
} | ||
|
||
.inputEmail { | ||
margin-bottom: 0px; | ||
border-radius: 10px; | ||
outline-width: 0; | ||
width: 350px; | ||
padding: 0.4rem; | ||
} | ||
.inputPassword { | ||
margin-bottom: 0px; | ||
border-radius: 10px; | ||
outline-width: 0; | ||
width: 350px; | ||
padding: 0.4rem; | ||
margin-top: 20px; | ||
} |
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.
We're using CSS. Make sure the entire code is less than 6 lines.
@praveenscience |
src/components/App.js
Outdated
@@ -38,7 +38,7 @@ const App = () => { | |||
const SetSection = (Section, Content) => { | |||
setAppState({ | |||
...appState, | |||
Resume: { ...appState.Resume, [Section]: Content } | |||
Resume: { ...appState.Resume, [Section]: Content }, |
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.
Configure prettier to not have trailing commas.
src/components/App.js
Outdated
Set: !appState.Resume.About.ProfilePicture.Set, | ||
}, | ||
}, | ||
}, | ||
}); |
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.
Configure prettier to not have trailing commas.
src/components/App.js
Outdated
ProfileImage: reader.result, | ||
}, | ||
}, | ||
}, |
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.
Configure prettier to not have trailing commas.
|
||
const Header = ({ ChangePage, CurrentPage, Resume }) => { | ||
return ( | ||
<nav className="navbar navbar-expand navbar-dark justify-content-between rezume-header"> | ||
<span className="navbar-brand mb-0 h1 col-1">Rezume</span> | ||
<ul className="navbar-nav"> | ||
{Object.keys(Resume).map(NavItem => ( | ||
{Object.keys(Resume).map((NavItem) => ( |
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.
Configure prettier to not have extra parentheses.
<Route exact path="/" element={<App />}></Route> | ||
<Route path="/login" element={<Login />}></Route> | ||
<Route path="/signup" element={<Signup />}></Route> |
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 use component
instead of element
and self close the <Route />
?
.inputEmail { | ||
margin-bottom: 0px; | ||
border-radius: 10px; | ||
outline-width: 0; | ||
width: 350px; | ||
padding: 0.4rem; | ||
} | ||
.inputPassword { | ||
margin-bottom: 0px; | ||
border-radius: 10px; | ||
outline-width: 0; | ||
width: 350px; | ||
padding: 0.4rem; | ||
margin-top: 20px; | ||
} |
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.
Better CSS?
margin-left: 1rem; | ||
border-radius: 10px; | ||
width: 100px; | ||
outline-width: 0; | ||
margin-right: 20px; | ||
margin-top: 20px; |
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.
Better and concise CSS?
.loginform { | ||
background-color: #003100; | ||
justify-content: center; | ||
padding: 2rem; | ||
text-align: center; | ||
align-items: center; | ||
height: 400px; | ||
margin: 10rem; | ||
} |
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.
Put the SCSS in the right directory.
@@ -0,0 +1,73 @@ | |||
import React from "react"; | |||
import "./index.scss"; |
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.
Put the SCSS in the right directory and do not import it this way.
@@ -0,0 +1,58 @@ | |||
import React from "react"; | |||
import "./index.scss"; |
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.
Put the SCSS in the right directory and do not import it this way.
hey @praveenscience |
hey @praveenscience ? |
Issue that this pull request solves
Closes: #117
Proposed changes
Added Login and Signup pages.
Types of changes
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that applyScreenshots
Please attach the screenshots of the changes made in case of change in user interface