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

Frontend login form #110

Open
wants to merge 8 commits into
base: frontend
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion ui/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@
"build-storybook": "build-storybook"
},
"dependencies": {
"leaflet": "^1.9.3",
"classnames": "^2.3.2",
"leaflet": "^1.9.3",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-leaflet": "^4.2.0",
"react-icons": "^4.7.1",
"stylelint-config-prettier-scss": "^0.0.1"
"react-leaflet": "^4.2.0",
"stylelint-config-prettier-scss": "^0.0.1",
"yup": "^1.1.1"
},
"devDependencies": {
"@babel/core": "^7.21.0",
Expand Down
Binary file added ui/src/assets/images/logo/pngs/logo_accent.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
20 changes: 20 additions & 0 deletions ui/src/assets/images/logo/svgs/logo_accent.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion ui/src/components/Button/Button.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Button.propTypes = {
/** color variant */
variant: PropTypes.oneOf(buttonVariants),
/** event that will be triggered when the button is clicked */
onClick: PropTypes.func.isRequired,
onClick: PropTypes.func,
/** content that will be displayed in the button */
children: PropTypes.string.isRequired,
/** additional css styles */
Expand Down
125 changes: 125 additions & 0 deletions ui/src/components/LoginForm/LoginForm.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
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_accent.svg'
import classNames from 'classnames'

export default function LoginForm() {
const emailOrNickInput = useRef(null)
const passwordInput = useRef(null)

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) => {
Copy link
Collaborator

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żdy onChange 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ć refy 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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Myślę, że teraz, gdy korzystamy z useRef, to ta funkcja nie jest nam za bardzo potrzebna 😉 Walidacja razem z ustawieniem błędów powinna się odbywać tylko po naciśnięciu przycisku. Po zmianie wartości w inputach nie potrzebujemy na bieżąco kontroli nad wartościami wpisywanymi przez użytkownika. Wprowadza to tylko niepotrzebne operacje 😄

Copy link
Member Author

Choose a reason for hiding this comment

The 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ł

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 } = event.target
if (errors[name]) {
setErrors((prev) => {
const newErrors = { ...prev }
delete newErrors[name]
return newErrors
})
}
setLoginError(false)
}

const handleSubmit = (event) => {
event.preventDefault()
schema
.validate(
{
emailOrNick: emailOrNickInput.current.value,
password: passwordInput.current.value
},
{
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}>
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 LoginForm.jsx był sam element form. Na razie niech tak to zostanie, ja się postaram ty zająć 😉

<header className={styles.viewHeader}>
<img src={logo} alt="logo" className={styles.logo} />
<div>
<h3 className={styles.header__level3}>Nie masz jeszcze konta?</h3>
<Button
style={classNames(styles.button, styles.registerButton)}
onClick={() => {
// TODO: change view to register
}}
>
Zarejestruj się
</Button>
</div>
</header>
<form className={styles.form} ref={loginForm} onSubmit={handleSubmit}>
<h3 className={styles.header__level3}>Zaloguj się do swojego konta</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think now is better but maybe we should avoid addicting to structure and only write just header or BEM element login__header?

Copy link
Collaborator

Choose a reason for hiding this comment

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

login__header sounds better 😄 header__level3 is a little bit confusing

<div className={styles.inputGroup}>
<label htmlFor="emailOrNick">Email/nick</label>
<input
type="text"
id="emailOrNick"
placeholder="[email protected] / Gigachad"
name="emailOrNick"
onChange={handleChange}
className={errors.emailOrNick && styles.errorInput}
ref={emailOrNickInput}
autoFocus
/>
{errors.emailOrNick && (
<span className={styles.errorSpan}>{errors.emailOrNick}</span>
)}
</div>

<div className={styles.inputGroup}>
<label htmlFor="password">Hasło</label>
<input
type="password"
id="password"
placeholder="********"
name="password"
onChange={handleChange}
className={errors.password && styles.errorInput}
ref={passwordInput}
/>
{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={classNames(styles.button, styles.submitButton)}>
Zaloguj się
</Button>
</form>
</div>
)
}
107 changes: 107 additions & 0 deletions ui/src/components/LoginForm/LoginForm.module.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
@use '../../assets/styles/abstracts' as *;

$form-gap: 1.875rem;
$password-reset-offset: -$form-gap;

.view {
min-height: 100vh;
box-sizing: border-box;
display: grid;
grid-template-rows: auto 1fr;
align-items: center;
background-color: #282828;
color: $base-white;
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any var for font weight? @marcol13 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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. 400 - normal, 600 - semibold.

What do you think about it? Should I add proper tokens for weight? @A-Wodnicki @PixelSculptor

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: 0.375rem solid $primary-p700;
border-radius: 0.625rem;
padding: 2.875rem 5.4375rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it expected to use these specific float values? @marcol13

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 rem


@media screen and (height > 720px) {
grid-row: 1 / -1;
}
}

.header__level3 {
font-weight: 600;
font-size: calc($font-size-h3 * 1px);
.viewHeader & {
display: inline;
}
.form & {
margin-bottom: 1.875rem;
}
}

button.button {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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 !important

Copy link
Contributor

Choose a reason for hiding this comment

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

have you tried .form__button?

Copy link
Collaborator

Choose a reason for hiding this comment

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

.form__button is more readable than button.button 😉

background-color: $primary-p500;
color: $alternative-a900;
font-weight: 600;
border-radius: $border-radius-md;
padding: 0.5625rem 1.875rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

same specific value


&.registerButton {
font-size: calc($font-size-button-md * 1px);
margin-left: 1.5rem;
}

&.submitButton {
font-size: calc($font-size-button-lg * 1px);
justify-self: center;
min-width: calc(2 / 3 * 100%);
}
}

.inputGroup {
display: grid;

input {
font-size: inherit;
padding: 0.8125rem 1.1875rem;
}
}

.inputGroup,
.passwordReset {
text-align: left;
}

.error {
&Span {
color: $communicates-error;
}

&Input {
outline: solid $communicates-error;
}
}

.passwordReset {
margin-top: $password-reset-offset;
text-decoration: none;
color: $base-white;
}

.logo {
max-width: 20rem;
}
15 changes: 15 additions & 0 deletions ui/src/components/LoginForm/LoginForm.stories.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import React from 'react'
Copy link
Collaborator

Choose a reason for hiding this comment

The 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'
Loading