Внесены изменения в ветку dev #7
Conversation
content.css
Outdated
| .slider-scrollbar .scrollbar-thumb { | ||
| width: 20%; | ||
| } | ||
| .video-container { | ||
| margin-top: 150px; | ||
| margin-bottom: 70px; | ||
| } | ||
|
|
||
| video { |
There was a problem hiding this comment.
по этому фрагменту кода видно что в лабораторной работе не используются средства автоматической проверки на соответствие гайдланам. Пожалуйста убедитесь в том что код отформатирован, проверен анализаторами. Подключите eslint/stylelint etc. Интегрируйте их в пайплайн проекта
content.html
Outdated
| <div class="slider-wrapper"> | ||
| <button id="prev-slide" class="slide-button material-symbols-rounded"> | ||
| chevron_left | ||
| </button> | ||
| <ul class="image-list"> | ||
| <img class="image-item" src="res/1.jpg" alt="img-1" /> | ||
| <img class="image-item" src="res/2.jpg" alt="img-2" /> | ||
| <img class="image-item" src="res/3.jpg" alt="img-3" /> | ||
| <img class="image-item" src="res/4.jpg" alt="img-4" /> | ||
| <img class="image-item" src="res/5.jpg" alt="img-5" /> | ||
| <img class="image-item" src="res/6.jpg" alt="img-6" /> | ||
| <img class="image-item" src="res/7.jpg" alt="img-7" /> | ||
| <img class="image-item" src="res/8.jpg" alt="img-8" /> | ||
| <img class="image-item" src="res/9.jpg" alt="img-9" /> | ||
| <img class="image-item" src="res/10.jpg" alt="img-10" /> | ||
| </ul> | ||
| <button id="next-slide" class="slide-button material-symbols-rounded"> | ||
| chevron_right | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
По сути, это компонент карусели, что значит что в теории ее можно вставить на страницу несколько раз. Однако сейчас это не так. Давайте сделаем карусель переиспользуемой. Для демонстрации добавьте еще одну карусель. с другими картинками, убедитесь что она тоже работает
content.html
Outdated
| chevron_left | ||
| </button> | ||
| <ul class="image-list"> | ||
| <img class="image-item" src="res/1.jpg" alt="img-1" /> |
There was a problem hiding this comment.
аlt - это не просто аттрибут, в который нужно вписать что-то уникальное. Пожалуйста, разберитесь в его назначении
package.json
Outdated
| "description": "[Ссылка на сайт](http://kolbaser.ru/)", | ||
| "main": "script.js", | ||
| "scripts": { | ||
| "lint:css": "stylelint --fix --ignore-path .gitignore *.css" |
script.js
Outdated
| const maxScrollLeft = imageList.scrollWidth - imageList.clientWidth; | ||
|
|
||
| scrollbarThumb.addEventListener("mousedown", (e) => { | ||
| const startX = e.clientX; |
There was a problem hiding this comment.
попробуйте подебажить ваше приложение и разобраться в проблеме.
поставьте тут console.log, сделайте ресайз окна несколько раз, далее нажмите на ползунок прокрутки.
Вы заметите что обработчик вызывается очень много раз.
Когда вы найдете решение проблемы - разберитесь с тем, как вы бы сами могли заметить эту проблему.
| uses: actions/checkout@v2 | ||
| - name: Install dependencies | ||
| run: npm install | ||
| - name: Lint CSS |
There was a problem hiding this comment.
Прекрасно; Не забудьте добавить так же линтер для js
|
Все замечания исправил. Оставил старые картинки, но разделил их между слайдерами |
| </head> | ||
|
|
||
| <body> | ||
| <div class="hero"> |
There was a problem hiding this comment.
Не очень понятна логика разбиения по файлам. из названия content.css складывается впечатление что его ответственность - стили для страницы content.html
Однако там определены стили для всех страниц.
Давайте наведем порядок, сделаем корректное деление
There was a problem hiding this comment.
Я перепроверил код по поводу этого комментария. У меня все четко распределено, т.е. один css к одному html файлу.
There was a problem hiding this comment.
Да, я ошибся. По всей видимости меня смутило
content.html content.css
НО
index.html style.css
index.html
Outdated
| <div class="hero"> | ||
| <div class="container hero-container"> | ||
| <div class="hero-about"> | ||
| <h1 class="hero_-title">Артем</h1> |
There was a problem hiding this comment.
Не совсем понятно по какой методологии вы пишите css классы.
На защиту предлагаю разобраться в методологии bem.
.stylelintrc.json
Outdated
| { | ||
| "extends": ["stylelint-config-standard"], | ||
| "rules": { | ||
| "selector-pseudo-class-no-unknown": null, | ||
| "selector-type-no-unknown": null, | ||
| "property-no-unknown": null, | ||
| "at-rule-no-unknown": null | ||
| } | ||
| } |
There was a problem hiding this comment.
По этому файлу видно что он не форматировался форматером автоматически. При этом валидация на CI это не заметила. Это значит что валидация все еще настроена не до конца. Давайте это исправим
|
Добавил форматирование с помощью Prettier без конфликта с ESLint и Stylelint |
script.js
Outdated
| let isListenerAdded = false | ||
|
|
||
| window.addEventListener('resize', () => { | ||
| isListenerAdded = false | ||
| }) |
There was a problem hiding this comment.
Для чего нужен этот флаг? вы его больше нигде не используете
| const scrollbarThumb = sliderScrollbar.querySelector('.scrollbar-thumb') | ||
| const maxScrollLeft = imageList.scrollWidth - imageList.clientWidth | ||
|
|
||
| scrollbarThumb.addEventListener('mousedown', (e) => { |
There was a problem hiding this comment.
При разработке компонентов/модулей, которые создают подписки на что-либо, является предоставление методов destroy/unmount/detach/dispose и тд которые выполняют отписку. Это важно, если потребуется добавлять/убирать компонент динамически. Выше есть тред где я указал на баг с ресайз. Вы пересоздавали компонент при ресайзе, а обработчики добавляли новые, не удаляя старые.
В конечном итоге вы переделали, на другой подход который я не понял, о чем оставил отдельный комментарий, но если вернуться к вашей предыдущей реализации, одно из решений - это перед новой иницилизацией - сделать destroy предыдущего объекта.
|
No description provided.