Ревью PR #97

Подход правильный — централизация селекторов с возможностью переопределения. Есть несколько проблем.

1. Определение направления кнопок +/- не использует селекторы (значимо)

В QuantityUI.handleButtonClick() направление определяется хардкодом:

const isInc = e.target.getAttribute('data-ms3-qty') === 'inc' || e.target.classList.contains('inc-qty')
const isDec = e.target.getAttribute('data-ms3-qty') === 'dec' || e.target.classList.contains('dec-qty')

Если пользователь переопределит qtyInc: '.my-plus-btn', кнопка получит listener (через initButtons), но isInc/isDec её не распознают — направление не определится. Нужно чтобы handleButtonClick использовал e.target.matches(qtyIncSel) / e.target.matches(qtyDecSel) вместо хардкода.

2. IE11 polyfill — мёртвый код

const matches = form.matches || form.msMatchesSelector || form.webkitMatchesSelector

Весь код использует ?. (optional chaining), async/await, ...spread — ничего из этого не работает в IE11. Fallback на msMatchesSelector/webkitMatchesSelector никогда не выполнится. Достаточно просто form.matches(formSel).

3. .eslintrc.json — косметическое переформатирование

Не относится к issue #18. Единственное значимое изменение — добавление getSelectors в globals, остальное — шум в diff.

4. Повторяющийся паттерн (this.config?.selectors || {})

Эта конструкция встречается ~15 раз. Можно вынести в геттер или закешировать в конструкторе каждого UI-класса:

get sel () {
  return this.config?.selectors || {}
}

5. console.warn ссылается на внутренний конфиг

console.warn('ms3_link must be inside a form matching sel.form')

sel.form ничего не скажет разработчику при отладке. Лучше вывести реальный селектор:

console.warn(`ms3_link must be inside a form matching: ${formSel}`)

@biz87 можно смотреть

Ревью PR #97 (повторное)

Статус предыдущих замечаний

Новые замечания

1. .eslintrc.json — косметика всё ещё в PR (minor)

Из предыдущего ревью. Единственное значимое изменение — "getSelectors": "readonly", остальное — перенос extends/env на одну строку. Лишний шум в diff. Стоит откатить форматирование, оставив только добавление getSelectors.

2. Дублирование inline fallback-ов (средне)

Каждое место использования содержит fallback-строку, идентичную дефолту в Selectors.js:

// Selectors.js
form: '[data-ms3-form], .ms3_form',

// ms3.js:115
const formSelector = selectors.form || '[data-ms3-form], .ms3_form'

// CartUI.js:50
const formSelector = selectors.form || '[data-ms3-form], .ms3_form'

// QuantityUI.js:96
const formSelector = selectors.form || '[data-ms3-form], .ms3_form'
// ...и ещё ~15 мест

Это частично подрывает цель централизации — при изменении дефолта в Selectors.js нужно менять все inline fallback-и.

selectors мержатся в ms3.config при init(), и все UI-классы получают уже готовый конфиг с селекторами (строка 71-75 ms3.js). Если Selectors.js не загрузился, getSelectors fallback на Ms3DefaultSelectors fallback на {} — и тогда inline fallback-и спасают ситуацию. Однако вероятность этого сценария крайне мала (файл в том же списке assets и загружается синхронно).

Предложение: оставить fallback-и только в ms3.js:init() (единственная точка входа), а в UI-классах убрать || '...' — доверять конфигу. Это устранит дублирование без потери надёжности.

3. 'selectors' => [] в MiniShop3.php (minor)

'selectors' => []

PHP [] → JSON [] (массив). Семантически это map (ключ-значение), должен быть объект:

'selectors' => (object)[]  // → JSON {}

Функционально работает (spread пустого массива в JS ничего не делает), но для консистентности лучше (object)[] или new \stdClass().

Что хорошо

• Архитектура — правильное решение: дефолты + merge с пользовательскими значениями, единая точка конфигурации
• Обратная совместимость — дефолтные селекторы включают и data-ms3-*, и CSS-классы
• Миграция — идемпотентна, повторяет проверенный паттерн из add_product_card_ui / add_quantity_ui
• Дедупликация — убран ручной dedup через Set в QuantityUI/CartUI (querySelectorAll с составным селектором и так не дублирует)
• Геттеры — чистый паттерн get selectors() вместо повторения this.config?.selectors || {}
• Переименования — sel → selectors, cartCostEl → cartCostElement — читаемее
• Переход от getElementById к querySelector в OrderUI — необходим для поддержки переопределяемых селекторов

Вердикт

Approve с minor замечаниями. Замечания 1 и 3 — косметические, можно поправить перед мержем или после. Замечание 2 (inline fallback-и) — стоит обсудить, но не блокирует.