Открывается и закрывается #7
Conversation
keksobot
changed the title
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
There was a problem hiding this comment.
Твой код содержит довольно много комментариев, и это может считаться антипаттерном — когда избыточные пояснения делают код перегруженным и сложным для восприятия. Комментарии, объясняющие очевидные вещи, часто увеличивают объем кода, не добавляя реальной ценности.
Считается хорошей практикой писать самодокументируемый код, то есть выбирать такие имена переменных и функций, чтобы их смысл был ясен без дополнительных пояснений. Обычно комментарии оставляют только там, где логика сложная или требует объяснения, которое невозможно выразить через имена.
Если тебе сейчас так удобнее, и комментарии помогают ориентироваться, можешь оставить их. Со временем, когда почувствуешь себя увереннее, попробуй минимизировать комментарии, сохраняя только те, которые добавляют что-то действительно полезное к пониманию кода.
Ещё у тебя в файле js/functions.js остались console.log, на которые ругается ESLint, давай их уберем, чтобы не мешались
js/img-view.js
Outdated
| // удаляем класс modal-open, чтобы контейнер с фотографиями снова скроллился | ||
| document.body.classList.remove('modal-open'); | ||
| // удаляем с кнопки для закрытия окна просмотра обработчик закрытия по клику на нее | ||
| bigPictureCancelNode.removeEventListener('click', onBigPictureCancelClick); |
There was a problem hiding this comment.
Обрати внимание здесь и на 32 строке на ошибки от ESLint
There was a problem hiding this comment.
Я пытался вынести объявление этих функций до этого блока, но в них используется функция closeBigPicture - ругается на нее. Замкнутый круг. Или просто объявить их двумя строчками (это желательно делать или можно так оставить)?
По поводу комментариев. Я самостоятельно не смог сделать это дз. Даже посмотрев разбор, мне понадобилось много времени, чтобы понять логику. Собственно, комментировал каждый шаг именно для этого и чтобы ты проверил. Это было еще в сентябре, до перерыва. Сейчас по этим комментариям заново пытf.cm все это понять)
There was a problem hiding this comment.
Ты можешь изменить здесь стрелочные функции на function declaration, т.е. function closeBigPicture и т. д. Тогда благодаря хоистингу эта ошибка исчезнет
js/img-view.js
Outdated
| const onEscKeydown = (evt) => { | ||
| // проверяем, что нажата клавиша Esc | ||
| if (evt.key === 'Escape') { | ||
| // отменяем действие по умолчанию | ||
| evt.preventDefault(); | ||
| // вызываем функцию закрытия окна просмотра | ||
| closeBigPicture(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
❗️ Это не ошибка, просто для кругозора
Вместо проверки if (evt.key === 'Escape') и выполнения основной логики внутри, лучше сначала сделать проверку на противоположное условие — if (evt.key !== 'Escape') { return }.
Такой подход сразу отсеивает ненужные сценарии и позволяет основной логике функции быть менее вложенной, что делает её проще для чтения и понимания.
| const onEscKeydown = (evt) => { | |
| // проверяем, что нажата клавиша Esc | |
| if (evt.key === 'Escape') { | |
| // отменяем действие по умолчанию | |
| evt.preventDefault(); | |
| // вызываем функцию закрытия окна просмотра | |
| closeBigPicture(); | |
| } | |
| }; | |
| const onEscKeydown = (evt) => { | |
| // если нажата не клавиша Esc, выходим из функции | |
| if (evt.key !== 'Escape') { | |
| return; | |
| } | |
| // отменяем действие по умолчанию | |
| evt.preventDefault(); | |
| // вызываем функцию закрытия окна просмотра | |
| closeBigPicture(); | |
| }; |
Использование early return часто помогает избавиться от лишней вложенности, особенно в больших функциях, делая их структуру более плоской и легко читаемой
js/img-view.js
Outdated
| pictureBlock.addEventListener('click', (evt) => { | ||
| const currentBigPicture = evt.target.closest('.picture'); | ||
| // проверяем, что кликнули по миниатюре | ||
| if (currentBigPicture) { | ||
| // аргументом передаем в функцию открытия полноэкранного изображения id изображения, по которому кликнули | ||
| openBigPicture(currentBigPicture.dataset.pictureId); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Ты импортируешь pictureBlock здесь только для того, чтобы навесить обработчик, но сам элемент определяется в другом месте. В итоге, логика «подготовки» этого блока оказывается размазанной по двум файлам, что делает ее неочевидной. Лучше сделать рефакторинг так, чтобы код был более «цельным»: каждый блок логики завершённым, без необходимости переходить между файлами для понимания, где какая часть инициализируется.
There was a problem hiding this comment.
Перенести этот фрагмент в thumbnails.js?
There was a problem hiding this comment.
Да, например, и тогда img-view хорошо переименовать в что-то про big-picture (т.к. он только им и будет заниматься).
Кстати, тебе возможно будет проще, если просто на бумаге попробуешь нарисовать связи. Просто каждый файлик - это квадрат, а стрелочками туда-обратно между ними импорты/экспорты. И потом попробовать посмотреть, где можно упростить связи.
…bnails.js блок pictureBlock с обработчиком
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
js/open-big-picture.js
Outdated
| } | ||
|
|
||
| // функция открытия полноэкранного изображения | ||
| function openBigPicture (pictureId) { |
There was a problem hiding this comment.
Смотир, при проверке тоже указало на ошибку. У тебя функция объявлена, но не экспортируется (хотя ты её используешь рядом)
There was a problem hiding this comment.
Точно, забыл про экспорт. Думал, ошибка указывает, что функция не используется конкретно в этом модуле.
|
♻️ Я собрал ваш пулреквест. Посмотреть можно здесь. |
🎓 Открывается и закрывается
💥 https://htmlacademy-javascript.github.io/2558253-kekstagram-2/7/