#106 into [email protected] 🌵 add dropdown feature, add system option in scheme switcher, change header items, change scheme colors, fix hover styles #182
Conversation
…ange header items, change scheme colors, fix hover styles
| dropwdownTrigger.addEventListener('click', (event) => { | ||
| event.stopPropagation(); | ||
| const isDropdownActive = dropdownContent.className === 'dropdown_content_active'; | ||
|
|
||
| if (isDropdownActive) { | ||
| closeDropdown(); | ||
| } else { | ||
| openDropdown(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
хендлер тоже можно вынести в константу по аналогии с onClickOutsideListener
| if (newScheme === 'system') { | ||
| scheme = getSystemScheme(); | ||
| mediaDark.addEventListener('change', () => setScheme('system')); | ||
| } else mediaDark.removeEventListener('change', () => setScheme('system')); |
There was a problem hiding this comment.
в dropdown/index.js есть скобки у else для одной строки, а тут нет
| .vl { | ||
| height: 30px; | ||
| border-left: 0.5px solid var(--primary-glass); | ||
| } |
There was a problem hiding this comment.
как тэг hr - горизонтальная линия, vl - вертикальная
| <svg width="21" height="21" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <title>npm</title> | ||
| <path fill="currentColor" d="M0.780029 0.469971V20.47H20.78V0.469971H0.780029ZM17.4269 17.1185H14.0738V7.17452H10.7207V17.1185H4.13145V3.82309H17.4269V17.1185Z" /> | ||
| </svg> |
| function setActiveRadioGroupItem(element) { | ||
| for (let i = 0; i < radioGroupItems.length; i += 1) { | ||
| radioGroupItems[i].className = 'scheme_switcher_radio_group_item'; | ||
| } | ||
| element.className = 'scheme_switcher_radio_group_item active'; | ||
| } |
There was a problem hiding this comment.
arrow declaration и forEach/for of не поддерживаются я так понимаю?
MiaInturi
added
documentation
| // eslint-disable-next-line no-undef | ||
| const { scheme, setScheme } = initScheme(); | ||
| // eslint-disable-next-line no-undef | ||
| const { closeDropdown } = initDropdown('scheme_switcher_dropdown'); |
There was a problem hiding this comment.
Я думаю надо для вьюшек сделать отдельный eslint конфиг, все равно же вьюшки в отдельной папке
src/static/views/pages/404/index.ejs
Outdated
| @@ -15,6 +15,7 @@ | |||
| <% const rootPath = (path) => `../../${path}` %> | |||
| <%- include(rootPath('features/scheme/index')) -%> | |||
| <%- include(rootPath('features/tab/index')) -%> | |||
| <%- include(rootPath('components/dropdown/index')) -%> | |||
There was a problem hiding this comment.
dropdown находится в папке features, а не в components
| <link rel="stylesheet" href="/components/dropdown/index.css" /> | ||
| <script defer src="/components/dropdown/index.js"></script> |
There was a problem hiding this comment.
dropdown находится в папке features, а не в components
| // eslint-disable-next-line no-unused-vars | ||
| function initDropdown(dropdownId) { | ||
| const dropdown = document.getElementById(dropdownId); | ||
| const dropwdownTrigger = document.getElementById(`${dropdownId}_trigger`); |
There was a problem hiding this comment.
опечатка в слове dropdown
There was a problem hiding this comment.
даже буквально с наводкой ещё тупила и не могла её разглядеть 🤦♂️
|
|
||
| svg { | ||
| color: var(--primary-glass); | ||
| font-weight: 400; |
There was a problem hiding this comment.
400 - это значение по умолчанию для font-weight, так что можно не писать этого
| function onClickOutside(element, handler) { | ||
| const onClickOutsideListener = (event) => { | ||
| if (!element.contains(event.target)) { | ||
| handler(); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('click', onClickOutsideListener); | ||
| } |
There was a problem hiding this comment.
Сейчас эта функция вызывается на каждый клик куда-либо (на строчке 36 эта функция является обработчиком кликов), но при этом же в этой функции потом снова идет подписка на клик. Т.е. каждый клик сейчас генерит новый eventListener, что не круто. Можно сделать проще. Тогда 1 подписка на ивент клика будет работать как надо
| function setActiveRadioGroupItem(element) { | ||
| for (let i = 0; i < radioGroupItems.length; i += 1) { | ||
| radioGroupItems[i].className = 'scheme_switcher_radio_group_item'; | ||
| } | ||
| element.className = 'scheme_switcher_radio_group_item active'; | ||
| } |
There was a problem hiding this comment.
Предлагаю в этом файле всю логику с добавлением и удалением css-классов сделать через свойство classList (https://developer.mozilla.org/en-US/docs/Web/API/Element/classList). Так будет более явно имхо. То же самое относится к строчке 20 и в целом ко всем прямым манипуляциям со свойством className
| if (newScheme === 'system') { | ||
| scheme = getSystemScheme(); | ||
| mediaDark.addEventListener('change', () => setScheme('system')); | ||
| } else mediaDark.removeEventListener('change', () => setScheme('system')); |
There was a problem hiding this comment.
removeEventListener находит функцию для удаления по ссылке, так что и в addEventListener и в removeEventListener надо прокидывать одну и ту же функцию, созданную заранее
| function initScheme() { | ||
| setScheme(getSavedScheme() ?? 'dark'); | ||
| const scheme = getSavedScheme() ?? 'system'; |
There was a problem hiding this comment.
Можно функцию вместо getSavedScheme сделать функцию getCurrentScheme и уже в ней будет логика ?? 'system' - чтобы определение схемы было полностью в одном месте
|
|
||
| if (newScheme === 'system') { | ||
| scheme = getSystemScheme(); | ||
| mediaDark.addEventListener('change', () => setScheme('system')); |
There was a problem hiding this comment.
сейчас listener вызовет снова setScheme, который снова поставит новый listener. И новые listener-ы будут добавляться каждый раз, когда системная тема меняется
| function setSchemeMedia(scheme) { | ||
| const lightMedia = scheme === 'light' ? 'all' : 'not all'; | ||
| const darkMedia = scheme === 'dark' ? 'all' : 'not all'; | ||
|
|
||
| lightStyles.media = lightMedia; | ||
| darkStyles.media = darkMedia; |
There was a problem hiding this comment.
Еще мне не очень нравится текущая реализация смены темы через изменения media (как минимум потому что это триггерит загрузку файлов каждый раз при смене темы). Я считаю, что проще обойтись одним css-файлом
И при смене темы просто добавлять/удалять класс dark_theme. Ну или по умолчанию сделать темным, а добавлять/удалять light_theme - тут уже без разницы. И тогда весь css будет в одном месте и не будут делаться лишние запросы.
Темная тема:
Светлая тема:
