Code review #1
Code review #1
Conversation
static/upload_module/template.html
Outdated
| <body> | ||
|
|
||
| <p>Здесь HTML-код компонента.</p> | ||
| <div class="module__upload"></div> |
There was a problem hiding this comment.
Инициализация модуля через один <div> элемент - это прекрасное минималистичное решение. Но просьба выбрать более специфичное имя для базового класса, например: multiple-issues-upload.
Плюс к этому, если используется методология БЭМ, то класс module__upload как "элемент" обязан находиться внутри элемента с классом module как "блока". Так что предлагаю либо отказаться от использования БЭМ в принципе (для такого маленького проекта он не нужен), либо пересмотреть нейминг: https://cssguidelin.es/#bem-like-naming
There was a problem hiding this comment.
Стоит добавить один дата-атрибут для определения числа выпусков в комиксе: data-issues-count чтобы правильно нумеровать новые страницы.
static/upload_module/template.html
Outdated
| <link rel="stylesheet" href="style.css"> | ||
| <link rel="stylesheet" href="modal.css"> |
There was a problem hiding this comment.
Если можно, это лучше объединить в один файл, чтобы модуль было проще подключать.
static/upload_module/main.js
Outdated
| @@ -0,0 +1,290 @@ | |||
| (function () { | |||
There was a problem hiding this comment.
Если везде используются стрелочные функции, то и тут можно.
static/upload_module/main.js
Outdated
| @@ -0,0 +1,290 @@ | |||
| (function () { | |||
| const Page = ` | |||
There was a problem hiding this comment.
Нейминг. Константы (не путать с константными переменными) в программировании называются капсом, например: PAGE_HTML_TEMPLATE;
static/upload_module/main.js
Outdated
| <a href="#" class="upload__card__button upload__card__button_move-left upload__card__button_move-left-${index}"> | ||
| <img class="image-fit" src="images/left-arrow.svg" alt="Передвинуть влево"> | ||
| </a> | ||
| <h5 class="upload__card__title">${name}</h5> |
There was a problem hiding this comment.
${name} - это потенциальное место для XSS-атаки. Все данные, которые приходит от пользователя, нужно вставлять через https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText
static/upload_module/main.js
Outdated
| files_.push(createFile(elem)); | ||
| count++; | ||
| } | ||
| dropbox.classList.add("upload__dropbox-disabled"); |
There was a problem hiding this comment.
Зону загрузки точно нужно отключать после первой пачки файлов? А если автор решит закинуть еще что-то?
static/upload_module/main.js
Outdated
|
|
||
| const createFileCard = (elem, index) => { | ||
| const card = document.createElement("div"); | ||
| card.className = `upload__card upload__card-${index}`; |
There was a problem hiding this comment.
Кажется, класс upload__card upload__card-${index} нигде не используется. Я рекомендую сохранить индекс через дата-атрибут и использовать его дальше в коде (например, в обработчиках кнопок).
| return card; | ||
| } | ||
|
|
||
| const createFile = (file, name = "", description = "") => { |
There was a problem hiding this comment.
Кажется, параметры name = "", description = "" нигде не используются.
static/upload_module/main.js
Outdated
| formElement.setAttribute('action', '/action/manageAddIssue'); | ||
| formElement.enctype = 'multipart/form-data'; | ||
|
|
||
| const idComic = document.createElement('input'); |
There was a problem hiding this comment.
Нейминг. Переменные называются по правилам английского языка: в английском существительные ставятся в обратном порядке: например, идентификатор комикса - это "comicsId" а не "idComics"
| list.items.add(elem.file); | ||
| image.files = list.files; | ||
|
|
||
| image.setAttribute('data-limit', '2097152') |
There was a problem hiding this comment.
Размер стоит проверять в момент, когда автор изначально добавляет файлы.
Codestyle fix
Loading animation
2 participants
PR for code review