#106 into [email protected] 🌵 add dropdown feature, add system option in scheme switcher, change header items, change scheme colors, fix hover styles #182

kvelian · 2024-06-30T17:27:52Z

Темная тема:

Светлая тема:

…ange header items, change scheme colors, fix hover styles

RiceWithMeat · 2024-06-30T18:26:50Z

src/static/views/features/dropdown/index.js

+  dropwdownTrigger.addEventListener('click', (event) => {
+    event.stopPropagation();
+    const isDropdownActive = dropdownContent.className === 'dropdown_content_active';
+
+    if (isDropdownActive) {
+      closeDropdown();
+    } else {
+      openDropdown();
+    }
+  });


хендлер тоже можно вынести в константу по аналогии с onClickOutsideListener

RiceWithMeat · 2024-06-30T18:30:17Z

src/static/views/features/scheme/index.js

+  if (newScheme === 'system') {
+    scheme = getSystemScheme();
+    mediaDark.addEventListener('change', () => setScheme('system'));
+  } else mediaDark.removeEventListener('change', () => setScheme('system'));


в dropdown/index.js есть скобки у else для одной строки, а тут нет

RiceWithMeat · 2024-06-30T18:37:32Z

src/static/views/assets/styles/global.css

+.vl {
+  height: 30px;
+  border-left: 0.5px solid var(--primary-glass);
+}


что vl означает?

как тэг hr - горизонтальная линия, vl - вертикальная

RiceWithMeat · 2024-06-30T18:42:36Z

src/static/views/components/header/index.ejs

+            <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>


свгшки не сжаты

RiceWithMeat · 2024-06-30T18:52:06Z

src/static/views/components/header/index.js

+  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';
+  }


arrow declaration и forEach/for of не поддерживаются я так понимаю?

можно сделать

MiaInturi

Еще предлагаю задуматься над использованием JS модулей, т.к. сейчас весь js как бы в одном пространстве имен. Хотя браузеры давно поддерживают модули

MiaInturi · 2024-07-01T11:54:41Z

src/static/views/components/header/index.js

+  // eslint-disable-next-line no-undef
+  const { scheme, setScheme } = initScheme();
+  // eslint-disable-next-line no-undef
+  const { closeDropdown } = initDropdown('scheme_switcher_dropdown');


Я думаю надо для вьюшек сделать отдельный eslint конфиг, все равно же вьюшки в отдельной папке

MiaInturi · 2024-07-06T06:42:45Z

src/static/views/pages/404/index.ejs

@@ -15,6 +15,7 @@
  <% const rootPath = (path) => `../../${path}` %>
  <%- include(rootPath('features/scheme/index')) -%>
  <%- include(rootPath('features/tab/index')) -%>
+  <%- include(rootPath('components/dropdown/index')) -%>


dropdown находится в папке features, а не в components

MiaInturi · 2024-07-06T06:49:00Z

src/static/views/features/dropdown/index.ejs

+<link rel="stylesheet" href="/components/dropdown/index.css" />
+<script defer src="/components/dropdown/index.js"></script>


dropdown находится в папке features, а не в components

MiaInturi · 2024-07-06T06:50:13Z

src/static/views/features/dropdown/index.js

+// eslint-disable-next-line no-unused-vars
+function initDropdown(dropdownId) {
+  const dropdown = document.getElementById(dropdownId);
+  const dropwdownTrigger = document.getElementById(`${dropdownId}_trigger`);


опечатка в слове dropdown

даже буквально с наводкой ещё тупила и не могла её разглядеть 🤦‍♂️

MiaInturi · 2024-07-06T06:53:10Z

src/static/views/assets/styles/global.css

-
-svg {
-  color: var(--primary-glass);
+  font-weight: 400;


400 - это значение по умолчанию для font-weight, так что можно не писать этого

MiaInturi · 2024-07-06T07:51:50Z

src/static/views/features/dropdown/index.js

+function onClickOutside(element, handler) {
+  const onClickOutsideListener = (event) => {
+    if (!element.contains(event.target)) {
+      handler();
+    }
+  };
+
+  document.addEventListener('click', onClickOutsideListener);
+}


Сейчас эта функция вызывается на каждый клик куда-либо (на строчке 36 эта функция является обработчиком кликов), но при этом же в этой функции потом снова идет подписка на клик. Т.е. каждый клик сейчас генерит новый eventListener, что не круто. Можно сделать проще. Тогда 1 подписка на ивент клика будет работать как надо

MiaInturi · 2024-07-06T08:15:47Z

src/static/views/components/header/index.js

+  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';
+  }


Предлагаю в этом файле всю логику с добавлением и удалением css-классов сделать через свойство classList (https://developer.mozilla.org/en-US/docs/Web/API/Element/classList). Так будет более явно имхо. То же самое относится к строчке 20 и в целом ко всем прямым манипуляциям со свойством className

огнище, спасибо 🔥

MiaInturi · 2024-07-06T08:26:28Z

src/static/views/features/scheme/index.js

+  if (newScheme === 'system') {
+    scheme = getSystemScheme();
+    mediaDark.addEventListener('change', () => setScheme('system'));
+  } else mediaDark.removeEventListener('change', () => setScheme('system'));


removeEventListener находит функцию для удаления по ссылке, так что и в addEventListener и в removeEventListener надо прокидывать одну и ту же функцию, созданную заранее

MiaInturi · 2024-07-07T07:48:00Z

src/static/views/features/scheme/index.js

 function initScheme() {
-  setScheme(getSavedScheme() ?? 'dark');
+  const scheme = getSavedScheme() ?? 'system';


Можно функцию вместо getSavedScheme сделать функцию getCurrentScheme и уже в ней будет логика ?? 'system' - чтобы определение схемы было полностью в одном месте

MiaInturi · 2024-07-07T07:50:11Z

src/static/views/features/scheme/index.js

+
+  if (newScheme === 'system') {
+    scheme = getSystemScheme();
+    mediaDark.addEventListener('change', () => setScheme('system'));


сейчас listener вызовет снова setScheme, который снова поставит новый listener. И новые listener-ы будут добавляться каждый раз, когда системная тема меняется

MiaInturi · 2024-07-07T08:28:35Z

src/static/views/features/scheme/index.js

+function setSchemeMedia(scheme) {
  const lightMedia = scheme === 'light' ? 'all' : 'not all';
  const darkMedia = scheme === 'dark' ? 'all' : 'not all';

  lightStyles.media = lightMedia;
  darkStyles.media = darkMedia;


Еще мне не очень нравится текущая реализация смены темы через изменения media (как минимум потому что это триггерит загрузку файлов каждый раз при смене темы). Я считаю, что проще обойтись одним css-файлом

И при смене темы просто добавлять/удалять класс dark_theme. Ну или по умолчанию сделать темным, а добавлять/удалять light_theme - тут уже без разницы. И тогда весь css будет в одном месте и не будут делаться лишние запросы.

#106 🌵 add dropdown feature, add system option in scheme switcher, ch…

6990455

…ange header items, change scheme colors, fix hover styles

RiceWithMeat reviewed Jun 30, 2024

View reviewed changes

MiaInturi added documentation Improvements or additions to documentation enhancement New feature or request [email protected] issues for [email protected] labels Jul 1, 2024

MiaInturi linked an issue Jul 1, 2024 that may be closed by this pull request

Create routes page #106

Open

MiaInturi requested changes Jul 7, 2024

View reviewed changes

MiaInturi reviewed Jul 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#106 into [email protected] 🌵 add dropdown feature, add system option in scheme switcher, change header items, change scheme colors, fix hover styles #182

#106 into [email protected] 🌵 add dropdown feature, add system option in scheme switcher, change header items, change scheme colors, fix hover styles #182

kvelian commented Jun 30, 2024

RiceWithMeat Jun 30, 2024

RiceWithMeat Jun 30, 2024

RiceWithMeat Jun 30, 2024

kvelian Jul 1, 2024

RiceWithMeat Jun 30, 2024

RiceWithMeat Jun 30, 2024

debabin Aug 4, 2024

MiaInturi left a comment

MiaInturi Jul 1, 2024

MiaInturi Jul 6, 2024

MiaInturi Jul 6, 2024

MiaInturi Jul 6, 2024

kvelian Aug 9, 2024

MiaInturi Jul 6, 2024

MiaInturi Jul 6, 2024 •

edited

Loading

MiaInturi Jul 6, 2024

kvelian Aug 9, 2024

MiaInturi Jul 6, 2024

MiaInturi Jul 7, 2024

MiaInturi Jul 7, 2024

MiaInturi Jul 7, 2024

		<link rel="stylesheet" href="/components/dropdown/index.css" />
		<script defer src="/components/dropdown/index.js"></script>

#106 into [email protected] 🌵 add dropdown feature, add system option in scheme switcher, change header items, change scheme colors, fix hover styles #182

Are you sure you want to change the base?

#106 into [email protected] 🌵 add dropdown feature, add system option in scheme switcher, change header items, change scheme colors, fix hover styles #182

Conversation

kvelian commented Jun 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiaInturi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiaInturi Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiaInturi Jul 6, 2024 •

edited

Loading