-
Notifications
You must be signed in to change notification settings - Fork 0
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
Frontend login form #110
base: frontend
Are you sure you want to change the base?
Frontend login form #110
Changes from 4 commits
bf6617d
881d2a9
c4afbb2
9841b0d
d383515
c109728
8e6bbc3
8aa5a85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
import styles from './LoginForm.module.scss' | ||
import Button from '../Button/Button' | ||
import { useRef, useState } from 'react' | ||
import { object, string } from 'yup' | ||
import logo from '../../assets/images/logo/svgs/logo_white.svg' | ||
|
||
export default function LoginForm() { | ||
const [inputs, setInputs] = useState({ | ||
emailOrNick: '', | ||
password: '' | ||
}) | ||
|
||
const [errors, setErrors] = useState({}) | ||
|
||
const [loginError, setLoginError] = useState(false) | ||
|
||
const loginForm = useRef(null) | ||
|
||
const schema = object({ | ||
emailOrNick: string().required('Pole "Email/nick" jest wymagane.'), | ||
password: string().required('Pole "Hasło" jest wymagane.') | ||
}) | ||
|
||
const handleChange = (event) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Myślę, że teraz, gdy korzystamy z There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Czyli błędy, które wcześniej się pojawiły mają się chować przy submicie? Ta funkcja je chowa jak wpisze się coś do inputa, w którym błąd wystąpił There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aaa rozumiem... Nie przewidywaliśmy w sumie takiego zachowania inputa — myślę, że dla prostoty i optymalizacji nie trzeba tutaj usuwać komunikatu o błędzie po wpisaniu tam czegokolwiek. |
||
const { name, value } = event.target | ||
setInputs((prev) => ({ ...prev, [name]: value })) | ||
if (errors[name]) { | ||
setErrors((prev) => { | ||
const newErrors = { ...prev } | ||
delete newErrors[name] | ||
return newErrors | ||
}) | ||
} | ||
setLoginError(false) | ||
} | ||
|
||
const handleSubmit = (event) => { | ||
event.preventDefault() | ||
schema | ||
.validate(inputs, { abortEarly: false }) | ||
.then(() => { | ||
console.log('ok') | ||
// TODO: send data to server | ||
setLoginError(true) // if failed to login | ||
}) | ||
.catch((error) => { | ||
const newErrors = {} | ||
error.inner.forEach((error) => { | ||
newErrors[error.path] = error.message | ||
}) | ||
setErrors(newErrors) | ||
loginForm.current[Object.keys(newErrors)[0]]?.focus() | ||
}) | ||
} | ||
|
||
return ( | ||
<div className={styles.view}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Z tego diva w późniejszym etapie będzie trzeba utworzyć jakiś nowy komponent 😉 Tak, żeby w pliku |
||
<header className={styles.viewHeader}> | ||
<img src={logo} alt="logo" /> | ||
<div> | ||
<h3>Nie masz jeszcze konta?</h3> | ||
<Button | ||
style={styles.button} | ||
onClick={() => { | ||
// TODO: change view to register | ||
}} | ||
> | ||
Zarejestruj się | ||
</Button> | ||
</div> | ||
</header> | ||
<form className={styles.form} ref={loginForm} onSubmit={handleSubmit}> | ||
<header> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wydaje mi się, że w tym miejscu element |
||
<h3>Zaloguj się do swojego konta</h3> | ||
</header> | ||
<div className={styles.inputGroup}> | ||
<label htmlFor="emailOrNick">Email/nick</label> | ||
<input | ||
value={inputs.emailOrNick} | ||
type="text" | ||
id="emailOrNick" | ||
placeholder="[email protected] / Gigachad" | ||
name="emailOrNick" | ||
onChange={handleChange} | ||
className={errors.emailOrNick && styles.errorInput} | ||
autoFocus | ||
/> | ||
{errors.emailOrNick && ( | ||
<span className={styles.errorSpan}>{errors.emailOrNick}</span> | ||
)} | ||
</div> | ||
|
||
<div className={styles.inputGroup}> | ||
<label htmlFor="password">Hasło</label> | ||
<input | ||
value={inputs.password} | ||
type="password" | ||
id="password" | ||
name="password" | ||
placeholder="********" | ||
onChange={handleChange} | ||
className={errors.password && styles.errorInput} | ||
/> | ||
{errors.password && ( | ||
<span className={styles.errorSpan}>{errors.password}</span> | ||
)} | ||
</div> | ||
<a className={styles.passwordReset} href="#"> | ||
{/* TODO: change view to password reset */} | ||
Zapomniałem hasła | ||
</a> | ||
{loginError && ( | ||
<span className={styles.errorSpan}>Nie udało się zalogować</span> | ||
)} | ||
<Button | ||
style={styles.button} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Żeby nie było potem wątpliwości i definicji w SCSS There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Możesz nawet nie zwracać specjalnie uwagi na złe stylowanie tego komponentu, bo |
||
onClick={() => loginForm.current.onSubmit} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tutaj, tak jak mówiliśmy na spotkaniu, możesz zmienić definicję komponentu |
||
> | ||
Zaloguj się | ||
</Button> | ||
</form> | ||
</div> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
@use '../../assets/styles/abstracts' as *; | ||
|
||
$form-gap: 1.875rem; | ||
|
||
.view { | ||
min-height: 100vh; | ||
box-sizing: border-box; | ||
display: grid; | ||
grid-template-rows: auto 1fr; | ||
align-items: center; | ||
background-color: #282828; | ||
color: $base-white; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please mark as TODO to refactor in the future this calculation with mixin |
||
font-size: calc($font-size-p * 1px); | ||
font-weight: 600; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there any var for font weight? @marcol13 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, there are no variables for that. I am not sure if it is necessary, because in default font-weight values have specific meaning, e.g. What do you think about it? Should I add proper tokens for |
||
padding: 1rem 2rem; | ||
gap: 1.875rem; | ||
} | ||
|
||
.viewHeader { | ||
grid-row: 1; | ||
grid-column: 1; | ||
display: flex; | ||
justify-content: space-between; | ||
align-items: center; | ||
} | ||
|
||
.form { | ||
grid-column: 1; | ||
justify-self: center; | ||
min-width: 34rem; | ||
display: grid; | ||
gap: $form-gap; | ||
text-align: center; | ||
background-color: $alternative-a900; | ||
border: 6px solid $primary-p700; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest change border width to rem cause everywhere you're using rem so it's good idea to keep consistency |
||
border-radius: 0.625rem; | ||
padding: 2.875rem 5.4375rem; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it expected to use these specific float values? @marcol13 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some of these values were copied directly from our Figma if they were available to check, I set it to display the values in rem units There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay approve ;) we need to figure out on our meeting how to round up/down these values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can discuss that on our next meeting 😉 Because maybe not everything should be in |
||
|
||
@media screen and (height > 45rem) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure if resolution for media query should be define as rem unit. @marcol13 what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Kapson007 yes, you are right. It should be defined in |
||
grid-row: 1 / -1; | ||
} | ||
} | ||
|
||
h3 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if we are using BEM but imo its better to use class selectors and avoid tag selectors as much as possible to control specificity properly. |
||
font-weight: 600; | ||
font-size: calc($font-size-h3 * 1px); | ||
.viewHeader & { | ||
display: inline; | ||
} | ||
.form & { | ||
margin-bottom: 1.875rem; | ||
} | ||
} | ||
|
||
button.button { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you want raise specificity? I think here you don't need button selector There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Higher specificity was required to override some rules over button component without using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. have you tried There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
background-color: $primary-p500; | ||
color: $alternative-a900; | ||
font-weight: 600; | ||
border-radius: $border-radius-md; | ||
padding: 0.5625rem 1.875rem; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same specific value |
||
|
||
.viewHeader & { | ||
font-size: calc($font-size-button-md * 1px); | ||
margin-left: 1.5rem; | ||
} | ||
|
||
.form & { | ||
font-size: calc($font-size-button-lg * 1px); | ||
justify-self: center; | ||
min-width: calc(2 / 3 * 100%); | ||
} | ||
} | ||
|
||
.inputGroup { | ||
display: grid; | ||
} | ||
|
||
.inputGroup, | ||
.passwordReset { | ||
text-align: left; | ||
} | ||
|
||
.error { | ||
&Span { | ||
color: $communicates-error; | ||
} | ||
|
||
&Input { | ||
outline: solid $communicates-error; | ||
} | ||
} | ||
|
||
.passwordReset { | ||
margin-top: -$form-gap; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the future its better to create another var rather than using like that beacuse now it's not semantic - using var for gaps in place for margins. |
||
} | ||
|
||
a { | ||
text-decoration: none; | ||
color: $base-white; | ||
} | ||
|
||
img { | ||
max-width: 20rem; | ||
} | ||
|
||
input { | ||
font-size: inherit; | ||
padding: 0.8125rem 1.1875rem; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import React from 'react' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ogólnie to niekoniecznie potrzebne są nam w Storybooku całe widoki jako historyjki 😉 Bardziej nam będzie zależało, żeby tam znalazły się komponenty, ale kudosy za to, że to postawiłeś 😁 Myślę, że możesz tak to zostawić, ale w przyszłości nie jest wymagane, żebyś to aplikował dla całych stron 😉 |
||
import LoginForm from './LoginForm' | ||
import '../../assets/styles/index.scss' | ||
|
||
export default { | ||
component: LoginForm, | ||
parameters: { | ||
layout: 'fullscreen' | ||
} | ||
} | ||
|
||
const Template = (args) => <LoginForm {...args} /> | ||
|
||
export const Primary = Template.bind({}) | ||
Primary.storyName = 'LoginForm' |
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.
Ciekawe podejście. Powinno działać, ale lepiej użyć do tego
useRef
. Ten hook nie powoduje rerenderowania się elementu przy każdej zmianie (w tym przypadku na każdyonChange
któregokolwiek z inputów), a walidacja w czasie rzeczywistym nie jest nam potrzebna (wystarczy jedynie sprawdzenie poprawności po kliknięciu przycisku). Jest to więc bardziej wydajny sposób na poradzenie sobie z formularzem 😉https://levelup.gitconnected.com/react-forms-usestate-vs-useref-5cb584cc19fd
Według mnie lepszym podejściem byłoby zrobić
ref
y na każdy input i po kliknięciu przycisku (czyli na onSubmit) odczytywać wartości inputów poprzez.current.value
.https://dev.to/sobhandash/react-forms-and-useref-hook-4p1l
Errory mogą być wtedy nadal przechowywane jako
useState
(bo będą wymuszały rerenderowanie pewnego fragmentu komponentu — komunikatu o błędzie).