-
Notifications
You must be signed in to change notification settings - Fork 29
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
coded challenge the sign in pages #13
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.
need to do a small refactoring.
good job
src/components/App/App.js
Outdated
if (location.pathname === '/') { | ||
if (location.pathname === '/' || location.pathname === '/sign-in') { |
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.
maybe you should try to add sign-in component as a modal?
it could be a modal, that will be displayed on top of Landing page, without any blurry efects for the background.
Needs to create a Modal component, and to make is flexible for all of projects needs.
src/pages/sign-in/SignIn.jsx
Outdated
|
||
import styles from './SignIn.modules.scss' | ||
|
||
import { ReactComponent as Close } from '../../components/icons/icons-svg/close.svg' |
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.
import {CloseIcon} from '../../../path/to/Icon'
…ut, select and button component
…ed FormProvider, created the popup modals for biometric sign in
please rebase to master and check linting errors |
…ut, select and button component
…ed FormProvider, created the popup modals for biometric sign in
… text in form fields
src/pages/sign-up/SignUp.jsx
Outdated
|
||
const methods = useForm() | ||
const onFormSubmit = data => { | ||
console.log(data) |
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.
remove this line
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.
I am not sure if want you want me to do is to remove line 16, 17 and 18, because line 16 is necessary for the react-hook-form to work as it gives me access to methods like the handleSubmit which is used to submit the form, but I assumed its the console.log you wanted me to remove so I removed just that.
…or importing the assets and changing them to absolute paths, making the images for the social media icons an array
No description provided.