Skip to content

feat(web): add scroll-to-top button - #176

Closed
Hard-system wants to merge 1 commit into
midt-bg:mainfrom
Hard-system:feat/scroll-to-top
Closed

feat(web): add scroll-to-top button#176
Hard-system wants to merge 1 commit into
midt-bg:mainfrom
Hard-system:feat/scroll-to-top

Conversation

@Hard-system

Copy link
Copy Markdown
Contributor

Какво и защо

Този PR добавя функционалност и бутон "Към началото" (Scroll to Top) в основния лейаут (root.tsx), за да се подобри UX/UI изживяването на потребителите.

Потребителска история:
Като потребител на платформата СИГМА, често преглеждам дълги списъци с договори, институции и компании. Когато скролирам надолу, искам да имам удобен и бърз начин да се върна в началото на страницата, за да използвам основната навигация или търсачката, без да се налага да скролирам ръчно обратно нагоре.

Детайли по имплементацията:

  • Създаден е нов компонент ScrollToTop.tsx с плавно визуализиране при скролиране над 400px надолу.
  • Интегриран глобално в root.tsx.
  • Използвани са вградените дизайн токъни (var(--ink), var(--accent)) за визуална консистентност с editorial стила на СИГМА.
  • Осигурена е пълна достъпност с ARIA атрибути (aria-label) и фокус мениджмънт чрез клавиатура (tabIndex и focus-visible стилове).

Свързан issue

N/A (Самостоятелно UX/UI подобрение)

Вид промяна

  • fix — поправка на бъг
  • feat — нова функционалност
  • docs — документация
  • refactor / perf / style — без промяна в поведението
  • test / ci / build / chore — поддръжка

Как е тествано

  • Ръчно тестване: потвърдено е плавното скролиране, визуализацията и скриването на компонента.
  • Проверка за адаптивност на мобилни устройства.
  • Тестване за клавиатурна достъпност (Tab навигация, Enter активация и ясен контур на фокуса).

Чеклист

  • Комитите следват conventional commits и нямат Co-Authored-By: trailer
  • PR-ът е с един логически обхват и е от форк към midt-bg/sigma:main
  • pnpm typecheck минава
  • pnpm test (поне за засегнатите пакети) минава
  • pnpm lint е чисто
  • Няма комитнати тайни, .env* или .dev.vars
  • Документацията в docs/ е обновена, ако промяната го налага

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверих ScrollToTop.tsx: SSR-безопасен (всичкият window достъп е в useEffect/handler-и), listener cleanup присъства, { passive: true }, a11y-то е коректно (aria-label, aria-hidden/tabIndex вързани за видимостта), няма hydration mismatch (useState(false) съвпада със server-render-а). Чисто.

@ydimitrof

Copy link
Copy Markdown
Contributor

Проверката е завършена. Изходът по-долу е финалният ревю коментар.


Ревю на PR #176feat(web): add scroll-to-top button

Здравейте, @Hard-system! Благодаря за приноса и за подробното описание с потребителска история — прегледах промяната внимателно, с акцент върху сигурност, интегритет на данните и OWASP съответствие.

Обхват и съответствие с описанието

PR-ът е атомарен и точно съответства на описанието: нов компонент ScrollToTop.tsx, стилове в app.css и глобална интеграция в root.tsx. Няма issue за сравнение (N/A), което е коректно отбелязано. Няма scope creep.

Сигурност (OWASP / интегритет на данните)

Промяната е изцяло клиентски UI компонент и не разширява атакуващата повърхност:

  • Инжекции (A03): няма SQL, няма достъп до база данни, няма сървърен код, няма конкатенация на заявки. Нищо за проверка тук.
  • XSS (A03): няма dangerouslySetInnerHTML, няма интерполация на потребителски вход. SVG иконата е статична, а единственият текст (aria-label="Към началото") е литерал. Чисто.
  • Тайни (A02/A07): няма хардкоднати ключове, токени или credentials. Няма променени URL адреси или зависимости.
  • SSR/hydration: целият достъп до window е капсулиран в useEffect и в handler-и; useState(false) съвпада със server-render-а — няма hydration mismatch и няма злонамерен/обфускиран код.

Присъединявам се към наблюдението на @lyubomir-bozhinov: cleanup на listener-а е налице, използва се { passive: true }, а достъпността е коректна (aria-hidden и tabIndex са вързани за видимостта, така че скритият бутон отпада от tab реда и a11y дървото). Няма съмнения по сигурността.

Блокиращ проблем — CI lint ще падне

Единствената пречка за merge е форматиране. prettier --check пада върху apps/web/app/app.css (потвърдено локално), а lint вече е блокиращ в CI (commit 2d93cd5). Prettier 3.x изисква multi-value свойствата да са разбити по редове:

  transition:
    opacity 0.3s ease,
    visibility 0.3s ease,
    transform 0.3s ease,
    background 0.2s ease;
  box-shadow:
    0 0 0 3px var(--paper),
    0 0 0 5px var(--accent);

ScrollToTop.tsx и root.tsx минават чисто — проблемът е само в app.css. Поправя се с:

pnpm exec prettier --write apps/web/app/app.css

В чеклиста pnpm lint, pnpm typecheck и pnpm test са необозначени — моля, изпълнете ги и потвърдете, преди повторно ревю.

Незадължителна препоръка (non-blocking)

Кодовата база вече уважава prefers-reduced-motion (виж app.css). За консистентност обмислете при prefers-reduced-motion: reduce да неутрализирате transition на .scroll-to-top и/или да ползвате behavior: 'auto' вместо 'smooth'. Не е пречка за merge.

Обобщение

Функционално и от гледна точка на сигурността промяната е чиста и добре изпълнена. Изисква се само форматиране, за да мине CI.

Вердикт: REQUEST_CHANGES — поправете Prettier форматирането в app.css (блокиращ lint); след това PR-ът е готов за одобрение.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Преглед на PR: feat(web): add scroll-to-top button

Обща оценка: добро качество, малка и добре обособена промяна. Компонентът е чист, използва window само вътре в useEffect (безопасно за SSR), закача scroll listener-а с { passive: true } и коректно го премахва при unmount. Достъпността е взета предвид (aria-label, aria-hidden, управление на tabIndex, :focus-visible стилове).

Phase 0 – Сигурност (сканиране)

  • Няма hardcoded тайни, ключове или токени.
  • Няма нови/променени URL адреси.
  • Няма нови зависимости.
  • Няма зловредни модели (backdoor, code injection, обфускация).
  • Резултат: CLEAN — не блокира.

Основни забележки

  1. Липсва поддръжка на prefers-reduced-motion (достъпност). И behavior: 'smooth' в JS, и transition в CSS игнорират системното предпочитание на потребителя за намалено движение. Това е WCAG препоръка и лесно се адресира.
  2. Няма тестове за новия компонент. Според изискванията за покритие (≥90% за нов код) трябва да се добави тест, който проверява превключването на видимостта при scroll над/под 400px и извикването на scrollTo.

Второстепенни

  • Прагът 400 и destination-ът на scroll са „magic“ стойности; ОК за такъв компонент, но може да се изнесат като константа.
  • Scroll handler-ът не е throttle-нат, но тъй като е passive и прави само евтина сравнителна проверка + условен setState, реалният performance ефект е пренебрежим.

Заключение

Функционалността е коректна и безопасна. Препоръчвам да се адресират prefers-reduced-motion и липсата на тестове преди merge. Verdict: COMMENT.

const scrollToTop = () => {
window.scrollTo({
top: 0,
behavior: 'smooth',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Достъпност: behavior: 'smooth' не зачита предпочитанието prefers-reduced-motion. Потребители, които са заявили намалено движение, ще получат анимирано превъртане. Препоръка: проверявайте window.matchMedia('(prefers-reduced-motion: reduce)').matches и подавайте behavior: 'auto' в този случай (или четете стойността динамично при клик).


useEffect(() => {
const toggleVisibility = () => {
if (window.scrollY > 400) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прагът 400 е „magic number". Препоръчително е да се изнесе като именувана константа (напр. SCROLL_THRESHOLD = 400) за четимост и по-лесна поддръжка. Освен това би било добре да се добави тест, който покрива превключването на isVisible при scrollY над/под прага, тъй като за новия компонент липсва тестово покритие.

Comment thread apps/web/app/app.css
opacity: 0;
visibility: hidden;
transform: translateY(16px);
transition: opacity 0.3s ease, visibility 0.3s ease, transform 0.3s ease, background 0.2s ease;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS transition също анимира появата/изчезването на бутона. За пълна съвместимост с prefers-reduced-motion добавете медия заявка @media (prefers-reduced-motion: reduce), която да намали/премахне тези преходи, съгласувано с промяната в JS-а.

@todorkolev

Copy link
Copy Markdown
Collaborator

Благодаря за PR-а, @Hard-system! Функционалността влезе в main чрез #201 - независима имплементация на същата идея, която добави и rAF throttle на scroll handler-а, prefers-reduced-motion поддръжка (JS + CSS) и позициониране, което не се застъпва с accessibility widget-а, плюс тестове. Идеята и първата реализация бяха твои - отбелязано е и в ревюто. Затварям като изместен от #201.

@todorkolev todorkolev closed this Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants