feat(web): productivity tools - copy citation button and print styles - #157
feat(web): productivity tools - copy citation button and print styles#157Hard-system wants to merge 5 commits into
Conversation
nedda76
left a comment
nedda76
left a comment
There was a problem hiding this comment.
Прегледах PR-а на два прохода (copy-citation + print стилове). Идеята е полезна, но има няколко реални бъга — част user-facing — които си струва да се затворят преди merge:
🔴 Цитатът за договор е счупен на ВСЯКА страница на договор. contract.tsx чете c.value.current и c.value.currency, но c.value (ContractValueTimeline) има currentEur/signingEur/estimatedEur — няма current, няма currency. Затова money(c.value.current) → „—", а ${c.value.currency} → буквалното „undefined". Всеки копиран цитат показва „Стойност: — undefined" вместо реалната сума (money() и без това добавя „€", та currency суфиксът е излишен). UI-ят по-горе ползва правилно v.currentEur.
🟠 Бутонът е без стил на екран. Класовете .save-btn / .is-saved / .save-btn-text / .header-actions нямат CSS извън @media print блока — няма базово .save-btn правило никъде. Print блокът само ги скрива; на екран (което виждат 99% от хората) бутонът е суров browser-default, до иначе стилизирания .source-cta. Добави стил или преизползвай .source-cta.
🟠 Няма .catch на clipboard write. navigator.clipboard.writeText(...).then(...) без .catch → при отказ (липса на фокус, отказано позволение, несигурен контекст) има unhandled rejection и нула обратна връзка — потребителят натиска „Копирай", нищо не става, не знае защо. Guard-ът за navigator.clipboard спира hard crash на HTTP, но тогава е тих no-op (без execCommand fallback).
🟡 Логиката за цитата е inline и дублирана в трите маршрута, без тест. Низът се сглобява на ръка в contract.tsx / company.tsx / authority.tsx. Репото има конвенция lib/*.test.ts точно за такава чиста логика — извади buildCitation(entity) и го покрий с тест (точно това щеше да хване №1; company/authority цитатите иначе са коректни).
🟡 Print: .facts-list е мъртъв селектор — компонентът рендира .facts, та break-inside: avoid за панела с ключови показатели е no-op и панелът пак се цепи през страниците.
🟡 Print скрива .source-cta — а линкът към официалния източник (ЦАИС ЕОП) е най-цитируемото на разпечатката; освен това никое print правило не извежда href-овете като текст, та хартиеното копие остава без използваеми URL-и.
🟡 Дребни: setTimeout(… setCopied(false)) не се чисти при unmount (setState-after-unmount, ако се навигира до 2s — а страниците са пълни с линкове); „Копирано!" е само визуално, без aria-live за екранни четци.
Хубаво: company/authority цитатите са добре оформени, линковете резолват правилно, CRLF е чист (LF), print правилата не текат към екрана, бутонът е keyboard-операбилен.
Идеята е добра — но бих помолила №1 (счупения цитат) и №2 (нестилизирания бутон) да се оправят преди merge, плюс .catch и изваден+тестван buildCitation. 🙏
f15858f to
63ad0ad
Compare
|
Благодаря за PR-а — функционалността е полезна и кодът е чист (SSR guard за Защо пада CI и как се оправяПадащата стъпка е
Другите два ( Стъпки:
Имай предвид, че Две неща за дооправяне1. ЕИК в цитата за компания заобикаля правилото за показване (важно за поверителност). Освен това профилите на единични физически лица са Предложение: подай 2. Линкът в цитата е hardcode-нат |
|
#157, #174 и #178 правят идентичния prettier reflow на (За #157 отделно, ако постваме: линковете в |
Преглед на PR #157 — Инструменти за продуктивност (copy-citation + print стилове)Благодаря за PR-а и за старателната работа по обратната връзка — прегледах текущото състояние ред по ред, с фокус върху сигурност, целостта на данните и съответствие с OWASP, и сверих имплементацията със заявеното в описанието. Всички основни забележки от предишните проходи вече са затворени. По-долу е обобщението. Сигурност (OWASP) — чисто
Целост на данните — коригирано
Съответствие с описаниетоИмплементацията покрива точно двете заявени неща — бутон „Копирай" на страниците за Договори, Компании и Институции, и изчистен изглед за печат. Няма разширяване на обхвата към логика или база. Дребни бележки (незадължителни)
Нищо от горните три не е свързано със сигурност или коректност на данните. Вердикт: ОДОБРЯВАМ — след премахване на извън-обхватните промени по |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: продуктивни инструменти — бутон за копиране на цитат и стилове за печат
Като цяло чист, добре структуриран PR с ясна цел. Логиката за изграждане на цитати (citation.ts) е изнесена като чисти функции и е добре покрита с unit тестове. Промените по CSS (header-actions, save-btn, print styles) са разумни и минимални. Не открих проблеми със сигурността: няма hardcoded secrets, няма нови зависимости, rel="noopener" е запазен за външния линк, а navigator.clipboard.writeText не носи риск от инжекция.
Забележка за средата: Прегледът е базиран само на diff-а. Няколко елемента не можах да проверя срещу реалния код (напр. дали getRootOrigin е експортиран от ../lib/meta, дали типът ContractDetail съдържа bidder.displayName/value.currentEur, и дали <main> елементът има id="main"). Моля, потвърдете ги при merge.
Основни констатации
- Липсват тестове за
CopyCitationButton.tsx— новият компонент съдържа нетривиална логика (clipboard, timeout cleanup,copiedсъстояние), но няма нито един тест. Според изискванията за покритие (≥90% за нов код) това е блокиращо. - Достъпност: потвърждението „Копирано!“ най-вероятно няма да бъде прочетено от екранни четци, защото
aria-labelна бутона потиска четенето на вътрешнияaria-liveрегион. - Print селекторът
#mainвероятно не съвпада с нищо (съществуващият CSS използваmain.narrow, т.е. клас, а не id). - Няколко дребни подобрения (тих провал при липсваща clipboard поддръжка,
font: inheritза<button>, дублиран fallback за origin).
Silno препоръчвам добавяне на тестове за компонента и коригиране на a11y проблема преди merge. Останалите са дребни. Оценка: ~8.5/10 — не одобрявам единствено заради липсата на тестове за новия компонент и достъпността.
| return ( | ||
| <button | ||
| type="button" | ||
| onClick={handleCopy} |
There was a problem hiding this comment.
Достъпност: Бутонът има статичен aria-label="Копирай данните като цитат". Когато aria-label е зададен, екранните четци използват него като достъпно име и обикновено не прочитат децата на бутона — включително aria-live="polite" спана на ред 68. Така промяната на текста към „Копирано!“ най-вероятно няма да бъде обявена на потребители с екранен четец, а именно това е целта на aria-live. Обмислете да преместите aria-live статуса в отделен визуално скрит елемент извън бутона, или да актуализирате aria-label/използвате aria-pressed, когато copied е true.
| }, []); | ||
|
|
||
| const handleCopy = useCallback(() => { | ||
| if (typeof navigator !== 'undefined' && navigator.clipboard) { |
There was a problem hiding this comment.
При липса на navigator.clipboard (несигурен контекст/HTTP, по-стари браузъри) кликът тихо не прави нищо — без обратна връзка към потребителя. Обмислете fallback (напр. document.execCommand('copy') или съобщение за грешка), за да не изглежда бутонът счупен.
| @@ -0,0 +1,75 @@ | |||
| import { useState, useCallback, useEffect, useRef } from 'react'; | |||
|
|
|||
| export function CopyCitationButton({ textToCopy }: { textToCopy: string }) { | |||
There was a problem hiding this comment.
Липсват тестове за този компонент. citation.ts е добре покрит, но CopyCitationButton съдържа нетривиална логика (успех/грешка при копиране, смяна на състоянието copied, изчистване на timeout при unmount), която не се проверява. Според изискванията за покритие (≥90% за нов код) добавете тестове (напр. с mock на navigator.clipboard и fake timers).
| } | ||
|
|
||
| /* Expand the content to fill the page width */ | ||
| #main, |
There was a problem hiding this comment.
Селекторът #main вероятно не съвпада с нито един елемент — останалата част от файла таргетира main.narrow (клас, не id). Ако <main> няма id="main", max-width: none при печат няма да се приложи. Моля потвърдете реалния селектор (вероятно main или main.narrow).
| /* Source CTA & Save Button — solid, icon-led actions. | ||
| Solid ink → accent on hover, matching the site's button language. */ | ||
| .source-cta, | ||
| .save-btn { |
There was a problem hiding this comment.
.save-btn вече е <button>, а не <a> като .source-cta. Браузърите не наследяват font-family/font-size за <button> по подразбиране, така че текстът на бутона може да изглежда различно от останалата типография на сайта. Добавете font: inherit; в този блок за визуална консистентност.
| } | ||
|
|
||
| /* Expose URLs in print */ | ||
| a[href]:not([href^='#'])::after { |
There was a problem hiding this comment.
a[href]:not([href^='#'])::after ще добави href-а и към вътрешните относителни линкове (напр. в таблиците), давайки частичен път като „ (/contracts/abc)“ вместо пълен URL — по-скоро шум, отколкото полза при печат. Обмислете да ограничите правилото само до абсолютни URL-и, напр. a[href^='http']::after.
|
|
||
| export default function Contract({ loaderData }: Route.ComponentProps) { | ||
| const matches = useMatches(); | ||
| const origin = getRootOrigin(matches) ?? 'https://sigma.midt.bg'; |
There was a problem hiding this comment.
Fallback-ът getRootOrigin(matches) ?? 'https://sigma.midt.bg' е повторен дословно в contract.tsx, company.tsx и authority.tsx. За да избегнете дублиране (и разминаване при бъдеща смяна на домейна), помислете да изнесете fallback стойността като константа/помощна функция до getRootOrigin в lib/meta.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах citation.ts + принт-стиловете. Кодът е чист: citation билдърите правят plain-text join към navigator.clipboard — няма HTML/innerHTML, тъй че няма XSS/инжекция; стойностите идват от типизирани данни, money()/count() са безопасни форматери; buildContractCitation чете current_value_eur (поправеното крак на #257), не наполовинения amount_eur. @media print скриването на nav/бутони/търсачка е презентационно, ниско-рисково.
Блокер за merge (не за кода): PR-ът е в конфликт (mergeable: dirty). Редактира монолитния app.css (ред ~676, .source-cta/main.narrow), но на main app.css вече е само @import манифест — тези правила са преместени в styles/*.css. Затова хункът не се прилага. Нужен е rebase: пренеси print-стиловете + новите .header-actions/.save-btn правила в съответния styles/*.css (вероятно components.css/pages.css), не в app.css.
Иначе фийчърът е издържан. След rebase-а върху текущата CSS структура може да върви.
|
Този клон е в конфликт с |
4 participants
Какво и защо
Този PR добавя Инструменти за продуктивност, които улесняват работата на разследващи журналисти, анализатори и граждани при извличане и споделяне на данни:
@media printстилове. Когато потребителят принтира страницата или я запазва като PDF (Ctrl+P), навигацията, бутоните и търсачката автоматично се скриват. Това генерира изчистен, официално изглеждащ документ и предпазва таблиците от "сцепване".Как е тествано
navigator.clipboard).