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

add: authorization (+ main) #5

Open
wants to merge 46 commits into
base: main
Choose a base branch
from
Open

add: authorization (+ main) #5

wants to merge 46 commits into from

Conversation

Alisa-it
Copy link
Collaborator

ПР "авторизация и регистрация" (+ merge с главной страницей)

@Alisa-it Alisa-it changed the title TP-10ee_auth add: authorization (+ main) Sep 29, 2024
Copy link
Collaborator

@erik770 erik770 left a comment

Choose a reason for hiding this comment

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

не забудьте добавить линтер!

README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
server.js Outdated Show resolved Hide resolved
server.js Outdated
});

app.post('/api/register', (req, res) => {
res.send(JSON.stringify({success: 'good'}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

{success: 'good'} убрать

Copy link
Collaborator

Choose a reason for hiding this comment

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

осталось success: 1, сомневаюсь что вам бекенд будет на каждый успешный запрос в теле ответа присылать success: number

server.js Outdated
res.send(JSON.stringify({success: 'good'}));
})

const cards = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

давайте сервер уберем в папку /server и в ней сделаем отдельный файл где будут лежать моковые данные

Copy link
Collaborator

Choose a reason for hiding this comment

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

моки в отдельный файл не убрали, файл сервера server.js можно в index.js

top:50%;
left:50%;
margin-top: -255px;
margin-left: -500px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

в 90% случаев если прибегаем к отрицательному марджину то что-то не так с версткой. тут как раз такой случай)

смотрите во первых у вас есть некий <div class="auth_wrapper" hidden></div> который непонятно зачем на странице
div.features div.auth хорошо бы как раз во враппер какой-нибудь засунуть и чтобы этому врапперу задать ширину высоту и соотношение в котором будут делиться div.features div.auth (это чтобы каждому из них не задавать width height)
еще заметил что .features .auth fixed хотя не должны бы, все это вынести в обертку

Copy link
Collaborator

Choose a reason for hiding this comment

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

еще вот у вас есть <div class="login_form active"> будто бы его можно использовать в качестве враппера а то тоже просто так висит пустой

font-family: 'logo';
font-size:36px;
margin-left: 43px;
margin-top: 57px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

давай

  1. не марджин а паддинг потому что это скорее внутренний отступ
  2. паддинг на весь контейнер

font-size: 16px;
align-items: center;
margin-top: 40px;
margin-left: 43px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

аналогично, марджины уйдут тк будет паддинг на контейнере + для отступов между элементами используй gap

src/components/auth/auth.css Outdated Show resolved Hide resolved
}

.authorization {
font-family: bold;
Copy link
Collaborator

Choose a reason for hiding this comment

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

#2 (comment)

font-family у вас у всех шрифтов кроме лого это Inter

в объявлении

@font-face {
    src: url(../fonts/Inter-Medium.otf);
}

указываем font-family: 'Inter'; font-weight и font-style, затем в местах где надо пожирнее или стилизовано оперируем font-weight и font-style

Copy link
Collaborator

@erik770 erik770 Sep 29, 2024

Choose a reason for hiding this comment

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

зря убрали

@font-face {
    font-family: medium;
    src: url(../fonts/Inter-Medium.otf);
}

@font-face {
    font-family: bold;
    src: url(../fonts/Inter-Bold.otf);
}

смотрите идея в чем: мы в font-face задаем некие параметры по типу font-family font-weight и font-style и для этих параметров соответствующий файл шрифтов откуда их брать

глобально у нас на body будет навешен font-family: Inter, some other fonts, etc. шрифт наследуется поэтому у всех элементов font-family будет такой, только если мы не переопределим его (как например для логотипа). а на конкретных элементах где нам допустим нужен жирный вариант шрифта нам будет достаточно написать font-weight 700, и применится шрифт из нужного файла ../fonts/Inter-Bold.otf потому что в @font-face будет указано что для font-weight 700 такой-то файл нужен

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

То есть, мы пишем:

@font-face {
    font-family: bold;
    font-weight: 700;
    src: url(../fonts/Inter-Bold.otf);
}

и при вызове font-weight: 700; он нам возвращает наш шрифт InterBold из файла а не ужирнённую версию InterRegular, так?

Copy link
Collaborator

Choose a reason for hiding this comment

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

агаа, именно так

Copy link
Collaborator

@erik770 erik770 left a comment

Choose a reason for hiding this comment

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

из глобального

  • надо бы сделать страничку 404 (очень желательно)
  • и задать разрешение мин на которое рассчитан текущий интерфейс и если оно меньше то показывать заглушку (необяз)

src/components/auth/auth.css Outdated Show resolved Hide resolved
font-size: 16px;
color: #FF0000;
margin-left: 71px;
margin-top: 15px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

аналогично все margin-left -> padding у контейнера, margin-top -> gap

Copy link
Collaborator

Choose a reason for hiding this comment

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

padding у контейнера

src/components/auth/auth.css Show resolved Hide resolved
background: url(../../static/images/eye_closed.svg);
}

.authorization_enter {
Copy link
Collaborator

Choose a reason for hiding this comment

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


.expand {
height: 570px;
margin-top: -285px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

опять же отрицательный марджин должен уйти

лучше всего сделать чтобы расширение происходило при изменении контента (при этом задать макс высоту после которой появится скролл). сама модалка оцентрована отосительно страницы

'Content-type': 'application/json',
}
});
const cards = await response.json();
Copy link
Collaborator

Choose a reason for hiding this comment

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

а если запрос на карточки упадет? или он будет идти 10 сек? у тебя хедера не будет в обоих случаях

  • можно если будет время заглушку поставить на шаблон, при загрузке лоадер а при ошибке сообщение об ошибке

src/pages/signup/signup.js Outdated Show resolved Hide resolved
const GET = 'GET';
const POST = 'POST';

export class Ajax {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ajax можно было в модулях оставить, вполне себе модуль

return await this.#handleResponse(response);
} catch (error) {
console.error('POST error:', error);
alert(error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

alert лучше убрать, хочется видеть ошибки на шаблоне а не нечитаемые ошибки
image

return re.test(email);
}

export function validatePassword(password) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

если будете успевать я бы добавил валидаций различных на пароль например

  • на наличие цифр
  • на символы

плюс щас нет проверки на фронте что повтор пароля совпадает с паролем (это уже не если будет время а обязательно)

const template = Handlebars.templates['auth.hbs'];
return template({title, info, inputs, buttontitle, pretext, anchortext});
const templateData = {title: data.title, info: data.info, inputs: data.inputs, buttontitle: data.buttontitle, pretext: data.pretext, anchortext: data.anchortext};
Copy link
Collaborator

Choose a reason for hiding this comment

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

поля вроде те же самые, можно без templateData обойтись

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants