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

DEAD-27: implement normal history api and url transitions #7

Open
wants to merge 9 commits into
base: DEAD-4-NEW
Choose a base branch
from

Conversation

AlexNov03
Copy link
Collaborator

No description provided.

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

Choose a reason for hiding this comment

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

обычно названия файлов в camelCase пишут

answer = response.body.data;
break;
default:
console.error('Error', response.status, response.error);
Copy link
Collaborator

@VladislavKirpichov VladislavKirpichov Oct 14, 2024

Choose a reason for hiding this comment

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

400+ методы должны нормально обрабатывать в приложении. Помню обсуждали на созвоне, ошибки тоже надо вытаскивать из респонса бекенда. И response.error - это ошибка сети а не апи.

Давай возвращать проверять боди и возвращать данные даже в случае ошибочных статусов. Посмотри что бекенд отдает сейчас на эту ручку вместе с 400/500

Comment on lines +1 to +9
export const ApiPaths = {
baseUrl: 'http://localhost:8000/api/v1',
user: {
login: '/login',
register: '/register',
logout: '/logout',
},
feed: '/feed',
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

переведи в UPPER_SNAKE_CASE, константы в нем принято писать

}

isValidEmail(email) {
var emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

почему var?

}

isValidPassword(password) {
const passwordRegex = /^[a-zA-Z0-9?!_\-*$]{6,}$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

нет ограничения на макс. кол-во символов + надо еще разрешать ( ) - + , . =. Вообще уменьшение кол-во возможных символов для ввода это плохая практика: https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#implement-proper-password-strength-controls


render() {
const placeForHeader = document.querySelector('.place-for-header');
placeForHeader.innerHTML = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

зачем сначала задавать '', а потом присваивать innerHTML = template? можно сразу присвоить template

Comment on lines +41 to +44
AuthHref.addEventListener('click', (event) => {
event.preventDefault();
Navigator.navigateTo('/auth');
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут может течь память так как листенеры надо чистить руками. Продумай логику так, чтобы колбеки сохранялись и вызывался removeEventListener при удалении компонента. Это всего кода касается

Comment on lines +56 to +57
const EmailInputVal = document.querySelector('#email-input').value.trim();
const PasswordInputVal = document.querySelector('#password-input').value.trim();
Copy link
Collaborator

Choose a reason for hiding this comment

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

trim лучше убери

Copy link
Collaborator

Choose a reason for hiding this comment

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

что-то лишнее

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