-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#106 into [email protected] 🌵 add dropdown feature, add system option in scheme switcher, change header items, change scheme colors, fix hover styles #182
base: [email protected]
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
хендлер тоже можно вынести в константу по аналогии с onClickOutsideListener
if (newScheme === 'system') { | ||
scheme = getSystemScheme(); | ||
mediaDark.addEventListener('change', () => setScheme('system')); | ||
} else mediaDark.removeEventListener('change', () => setScheme('system')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
в dropdown/index.js есть скобки у else для одной строки, а тут нет
.vl { | ||
height: 30px; | ||
border-left: 0.5px solid var(--primary-glass); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
что vl означает?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
как тэг 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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
свгшки не сжаты
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arrow declaration и forEach/for of не поддерживаются я так понимаю?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
можно сделать
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Еще предлагаю задуматься над использованием JS модулей, т.к. сейчас весь js как бы в одном пространстве имен. Хотя браузеры давно поддерживают модули
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я думаю надо для вьюшек сделать отдельный eslint конфиг, все равно же вьюшки в отдельной папке
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
опечатка в слове dropdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
даже буквально с наводкой ещё тупила и не могла её разглядеть 🤦♂️
|
||
svg { | ||
color: var(--primary-glass); | ||
font-weight: 400; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Сейчас эта функция вызывается на каждый клик куда-либо (на строчке 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Предлагаю в этом файле всю логику с добавлением и удалением css-классов сделать через свойство classList (https://developer.mozilla.org/en-US/docs/Web/API/Element/classList). Так будет более явно имхо. То же самое относится к строчке 20 и в целом ко всем прямым манипуляциям со свойством className
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
огнище, спасибо 🔥
if (newScheme === 'system') { | ||
scheme = getSystemScheme(); | ||
mediaDark.addEventListener('change', () => setScheme('system')); | ||
} else mediaDark.removeEventListener('change', () => setScheme('system')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeEventListener находит функцию для удаления по ссылке, так что и в addEventListener и в removeEventListener надо прокидывать одну и ту же функцию, созданную заранее
function initScheme() { | ||
setScheme(getSavedScheme() ?? 'dark'); | ||
const scheme = getSavedScheme() ?? 'system'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно функцию вместо getSavedScheme
сделать функцию getCurrentScheme
и уже в ней будет логика ?? 'system'
- чтобы определение схемы было полностью в одном месте
|
||
if (newScheme === 'system') { | ||
scheme = getSystemScheme(); | ||
mediaDark.addEventListener('change', () => setScheme('system')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
сейчас 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Еще мне не очень нравится текущая реализация смены темы через изменения media (как минимум потому что это триггерит загрузку файлов каждый раз при смене темы). Я считаю, что проще обойтись одним css-файлом
И при смене темы просто добавлять/удалять класс dark_theme. Ну или по умолчанию сделать темным, а добавлять/удалять light_theme - тут уже без разницы. И тогда весь css будет в одном месте и не будут делаться лишние запросы.
Темная тема:
Светлая тема: