TP: fix deploy #11
TP: fix deploy #11
Conversation
| res.sendFile(path.join(__dirname,'../src', 'index.html')); | ||
| }); | ||
|
|
||
| app.get('/seller/:id', (req, res) => { |
There was a problem hiding this comment.
можно использовать регулярку чтобы не дублироваться
There was a problem hiding this comment.
когда роутинг будет эти штуки все в одну сольются, так что наверное нет смысла сейчас что то править
There was a problem hiding this comment.
роутинг же будет именно на наше приложение а этот сервак отдает index.html. или тут тоже чето планируется?
src/components/advert/advert.hbs
Outdated
| <ol class="breadcrumbs__list"> | ||
| <li class="breadcrumbs__item"><a class="breadcrumbs__link" href="/">Главная</a></li> | ||
| <li class="breadcrumbs__item"><a class="breadcrumbs__link" href="{{categoryUrl}}">{{category}}</a></li> | ||
| <li class="breadcrumbs__item"><span class="breadcrumbs__link-not-active">{{title}}</span></li> |
There was a problem hiding this comment.
если уж по БЭМ то breadcrumbs__link-not-active not-active звучит как Модификтор
src/components/advert/advert.hbs
Outdated
| <img src="../../static/images/star.svg" class="seller__star"> | ||
| <img src="../../static/images/star.svg" class="seller__star"> | ||
| <img src="../../static/images/star.svg" class="seller__star"> | ||
| </span> --}} |
There was a problem hiding this comment.
коменты ненужные убираем тут и везде
src/components/advert/advert.js
Outdated
| async addComponent(wrapper, advertId) { | ||
| const advert = await ajax.get(`/adverts/${advertId}`); | ||
|
|
||
| const sellerId = await ajax.get(`/seller/${advert.seller_id}`); |
There was a problem hiding this comment.
у вас ручка на получение айдишника продавца по айдишнику объявления? показалось странным.. а почему это не приходит вместе с объявлением?
There was a problem hiding this comment.
а не, кажись тут просто надо переменные корректнее назвать
There was a problem hiding this comment.
у нас в запросе на объявление приходит айдишник селлера, по нему делаем запрос на селлера, чтобы получить айдишник юзера, с которым он ассоциирован, а по нему уже получаем имя, картинку и дату регистрации
There was a problem hiding this comment.
делаем запрос на селлера
а переменная куда записывается селлер называется sellerId
src/components/advert/advert.js
Outdated
| if(checkAuth()) { | ||
| me = await ajax.get('/me'); | ||
| if(advert.status === 'inactive' && seller.id !== me.id) { | ||
| return false; |
src/components/card/card.js
Outdated
|
|
||
| export function addCardListeners(cardsJSON) { | ||
| const cards = document.querySelectorAll('.card'); | ||
| for(let i = 0; i < cards.length; i++) { |
There was a problem hiding this comment.
итерируемся стандартными методами массивов
There was a problem hiding this comment.
да, я чето только сегодня увидел, что у forEach можно и элемент и индекс тянуть
src/components/card/card.js
Outdated
| const cards = document.querySelectorAll('.card'); | ||
| for(let i = 0; i < cards.length; i++) { | ||
| cards[i].addEventListener('click', () => { | ||
| window.location.href = `/advert/${cardsJSON[i].id}`; |
There was a problem hiding this comment.
а если cardsJSON будет отличаться от того что есть в верстке? тогда ссылки поедут. предлагаю навешивать слушатель при создании
src/components/cart/cart.hbs
Outdated
|
|
||
| {{#if notEmpty}} | ||
| <aside class="cart__buy"> | ||
| <div class="cart__items">1 товара<span class="cart__price">1₽</span></div> |
There was a problem hiding this comment.
почему количество и цена не являются переменными шаблона
| let totalCost = 0; | ||
| adverts.forEach(advert => { | ||
| let strCost = advert.querySelector('.adverts__price').innerText | ||
| totalCost += Number(strCost.slice(0, strCost.length - 1)); |
There was a problem hiding this comment.
ойойой это очень нехорошо, особенно тут, когда дело касается денег. это все должно рассчитываться на основе данных с бекенда а не на основе того что на шаблоне отрисовано. информация о цене товаров и их количестве же известна, почему бы не оперировать данными?
src/components/cart/cart.js
Outdated
| wrapper.querySelector('.cart__price').innerText = String(totalCost) + '₽'; | ||
| } | ||
|
|
||
| function getProductWord(quantity) { |
There was a problem hiding this comment.
вынести в функцию которую можно будет переиспользовать для других слов тоже. теги для поиска решений в гугле: pluralization js rus
There was a problem hiding this comment.
а еще почему где-то на функциях все а где-то на классах, приводим к 1 варианту все (предпочтительно классы)
There was a problem hiding this comment.
не уверен, что плюрализация может пригодиться где то еще
| @@ -0,0 +1,139 @@ | |||
| @mixin box-shadow-with-hover() { | |||
There was a problem hiding this comment.
если еще где-то есть такое то миксин можно вынести в отдельный файл с миксинами и подключать его по надобности
| } | ||
|
|
||
| trimNumber(number, maxLength) { | ||
| if(number.length > maxLength) { |
There was a problem hiding this comment.
проверка number.length > maxLength ни к чему, если maxLength больше то ничего не произойдет
| const categories = await ajax.get('/categories'); | ||
| for(const category of categories) { | ||
| if(category.Title === select.value) { | ||
| data['category_id'] = category.ID; |
There was a problem hiding this comment.
лучше чтобы в поле с инпутом мы сразу выбирали айдишник (отображаются названия а при выборе в пое хранится id). так тут нам не придется делать очередной запрос на категории
|
|
||
| const userId = localStorage.getItem('id'); | ||
|
|
||
| const seller = await ajax.get(`/seller/user/${userId}`) |
There was a problem hiding this comment.
а зачем этот запрос если ты из его результата используешь только seller.id? или seller.id это не тоже самое что userId
There was a problem hiding this comment.
да, у нас селлер и юзер это две разные сущности
| data['price'] = Number(data['price']); | ||
|
|
||
| let advert = {}; | ||
| if(updateAdvertId){ |
There was a problem hiding this comment.
а что тут происходит? что за updateAdvertId, логику не могу осознать
There was a problem hiding this comment.
поглядел по коду и не нашел чтобы с этим флагом метод вызывался, мб проглядел конечно
There was a problem hiding this comment.
у меня компонент createAdvert используется и для изменения, соответственно запрос будет не post, а put
| const image = imageInput.files[0]; | ||
| const formData = new FormData(); | ||
| formData.append('image', image); | ||
| await ajax.imagePut(`/adverts/${advert.id}/image`, formData); |
There was a problem hiding this comment.
а почему это делается отдельно а не в том же запросе что и создание? а если создание будет успешным а imagePut отвалится? получается пользователь подумает что ура объявление создалось и проблем нет но картинка будет отсутствовать
There was a problem hiding this comment.
на бэке отдельная ручка для загрузки картинок
No description provided.