From c8580752328286db2a80d97326e7ad2e1c5bbad1 Mon Sep 17 00:00:00 2001 From: fahed Date: Wed, 29 Apr 2026 09:41:38 +0300 Subject: [PATCH] refactor(report): full UX audit + accessibility pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UI/UX redesign: - Module cards with master toggle + badge state for all report sections - BreakdownModule with indeterminate checkbox and metric pill sub-toggles - PillGroup replaces all text toggles and onChange(e.target.checked)} className="rf-checkbox" /> - {label} - + onChange(e.target.checked)} className={className} /> + ); +} + +// C1: aria-hidden badge (visual only), role/aria on header label provides the accessible name +function ModuleCard({ title, badge, enabled, onToggle, children }: { + title: string; + badge?: string; + enabled: boolean; + onToggle: (v: boolean) => void; + children?: React.ReactNode; +}) { + return ( +
+ + {enabled && children && ( +
{children}
+ )} +
+ ); +} + +type MetricPatch = { revenue?: boolean; visitors?: boolean; tickets?: boolean }; + +function BreakdownModule({ title, revenue, visitors, tickets, onChange }: { + title: string; + revenue: boolean; visitors: boolean; tickets: boolean; + onChange: (patch: MetricPatch) => void; +}) { + const anyOn = revenue || visitors || tickets; + const allOn = revenue && visitors && tickets; + const badge = anyOn + ? [revenue && 'Revenue', visitors && 'Visitors', tickets && 'Tickets'].filter(Boolean).join(' · ') + : 'Excluded'; + + return ( +
+ + {anyOn && ( +
+ {/* C1+C3: role="group" + aria-label + aria-pressed */} +
+ {([ + { label: 'Revenue', on: revenue, key: 'revenue' as keyof MetricPatch }, + { label: 'Visitors', on: visitors, key: 'visitors' as keyof MetricPatch }, + { label: 'Tickets', on: tickets, key: 'tickets' as keyof MetricPatch }, + ]).map(({ label, on, key }) => ( + + ))} +
+
+ )} +
); } export default function ReportForm({ config: cfg, onChange, allMuseums, allChannels }: Props) { const logoInputRef = useRef(null); + // C2: inline error instead of alert() + const [logoError, setLogoError] = useState(null); const handleLogoUpload = (e: React.ChangeEvent) => { const file = e.target.files?.[0]; if (!file) return; - if (file.size > 2 * 1024 * 1024) { alert('Logo must be under 2 MB'); return; } + if (file.size > 2 * 1024 * 1024) { + setLogoError('File must be under 2 MB.'); + return; + } + setLogoError(null); const reader = new FileReader(); reader.onload = () => onChange({ clientLogoBase64: reader.result as string }); reader.readAsDataURL(file); @@ -56,115 +149,251 @@ export default function ReportForm({ config: cfg, onChange, allMuseums, allChann return (
+
- Client Info + {/* ── Left: setup ── */} +
+ {/* M2: semantic h2 instead of div — visually identical via CSS */} +

Client

- - onChange({ title: e.target.value })} - placeholder="Q1 2025 Visitor Performance" /> - + + onChange({ title: e.target.value })} + placeholder="Q1 2025 Visitor Performance" /> + - - onChange({ clientName: e.target.value })} - placeholder="Acme Group" /> - + + onChange({ clientName: e.target.value })} + placeholder="Acme Group" /> + - - onChange({ contactName: e.target.value })} - placeholder="Mohammed Al-..." /> - + + onChange({ contactName: e.target.value })} + placeholder="Mohammed Al-..." /> + - -
- - {cfg.clientLogoBase64 && ( - <> - preview - - +
+
+ Accent color +
+ onChange({ accentColor: e.target.value })} + className="rf-color-input" + aria-label="Report accent color" /> + {cfg.accentColor} +
+
+
+ Logo (PNG/JPG, max 2 MB) +
+ {/* H6: descriptive aria-label on upload button */} + + {cfg.clientLogoBase64 && ( + <> + {/* M1: meaningful alt text */} + Uploaded client logo + {/* H6: descriptive aria-label on remove button */} + + + )} + +
+ {/* C2: inline logo error */} + {logoError && {logoError}} +
+
+ +
+

Data

+ +
+ + onChange({ startDate: e.target.value })} /> + + + onChange({ endDate: e.target.value })} /> + +
+ + + onChange({ selectedMuseums: v })} + allLabel="All museums" countLabel={n => `${n} museums`} clearLabel="Clear" /> + + + + onChange({ selectedChannels: v })} + allLabel="All channels" countLabel={n => `${n} channels`} clearLabel="Clear" /> + + +
+ VAT + onChange({ includeVAT: v === 'incl' })} + /> +
+ + + + {cfg.includeComparison && ( +
+ +
+ + onChange({ comparisonStartDate: e.target.value })} /> + + + onChange({ comparisonEndDate: e.target.value })} /> + +
+
)} - + +
+

Format

+ +
+ Language + onChange({ language: v as 'en' | 'ar' })} + /> +
+ +
+ Orientation +
+ + +
+
+ +
+ Confidentiality + onChange({ confidentiality: v as ReportConfig['confidentiality'] })} + /> +
- - -
- onChange({ accentColor: e.target.value })} - className="rf-color-input" /> - {cfg.accentColor} + {/* ── Right: content selection ── */} +
+

Report Sections

+ + onChange({ showExecutiveSummary: v })} /> + onChange({ showMetricsTable: v })} /> + onChange({ showPilgrimCapture: v })} /> + +
+

Trend

+ + onChange({ showTrendChart: v })} + badge={cfg.showTrendChart + ? cfg.trendMetric.charAt(0).toUpperCase() + cfg.trendMetric.slice(1) + : undefined} + > + {/* H7: PillGroup instead of onChange({ startDate: e.target.value })} /> - - - onChange({ endDate: e.target.value })} /> -
- - - onChange({ selectedMuseums: v })} - allLabel="All museums" countLabel={n => `${n} museums`} clearLabel="Clear" /> - - - - onChange({ selectedChannels: v })} - allLabel="All channels" countLabel={n => `${n} channels`} clearLabel="Clear" /> - - - - onChange({ includeVAT: v })} /> - - - onChange({ includeComparison: v })} /> - - Content Sections - - onChange({ showExecutiveSummary: v })} /> - onChange({ showMetricsTable: v })} /> - onChange({ showTrendChart: v })} /> - onChange({ showMuseumBreakdown: v })} /> - onChange({ showChannelBreakdown: v })} /> - onChange({ showPilgrimCapture: v })} /> - - Presentation - - - onChange({ language: v ? 'ar' : 'en' })} /> - - - - onChange({ orientation: v ? 'landscape' : 'portrait' })} /> - - - - - -
); } diff --git a/src/components/Report/index.tsx b/src/components/Report/index.tsx index 8f4f8bb..4610105 100644 --- a/src/components/Report/index.tsx +++ b/src/components/Report/index.tsx @@ -1,11 +1,10 @@ -import React, { useState, useCallback } from 'react'; +import React, { useState, useCallback, useMemo, useEffect } from 'react'; import { pdf } from '@react-pdf/renderer'; import type { MuseumRecord } from '../../types'; import { DEFAULT_CONFIG, computeReportData } from './reportHelpers'; import type { ReportConfig } from './reportHelpers'; import { ReportDocument } from './ReportDocument'; import ReportForm from './ReportForm'; -import ReportPreview from './ReportPreview'; import { getUniqueMuseums, getUniqueChannels } from '../../services/dataService'; interface Props { @@ -15,18 +14,44 @@ interface Props { export default function ReportPage({ data }: Props) { const [config, setConfig] = useState(DEFAULT_CONFIG); const [generating, setGenerating] = useState(false); + const [errorMsg, setErrorMsg] = useState(null); - const allMuseums = getUniqueMuseums(data); - const allChannels = getUniqueChannels(data); + // H8: memoize — these scan the full records array; re-running on every patch is wasteful + const allMuseums = useMemo(() => getUniqueMuseums(data), [data]); + const allChannels = useMemo(() => getUniqueChannels(data), [data]); const patch = useCallback((p: Partial) => setConfig(c => ({ ...c, ...p })), []); + // C2: auto-clear inline error after 6 s + useEffect(() => { + if (!errorMsg) return; + const t = setTimeout(() => setErrorMsg(null), 6000); + return () => clearTimeout(t); + }, [errorMsg]); + + const sectionCount = useMemo(() => [ + config.showExecutiveSummary, + config.showMetricsTable, + config.showPilgrimCapture, + config.showTrendChart, + config.showMuseumRevenue || config.showMuseumVisitors || config.showMuseumTickets, + config.showChannelRevenue || config.showChannelVisitors || config.showChannelTickets, + config.showDistrictRevenue || config.showDistrictVisitors || config.showDistrictTickets, + config.showGlobalSummary && config.includeComparison, + ].filter(Boolean).length, [config]); + + const periodLabel = config.startDate && config.endDate + ? `${config.startDate.slice(0, 7)} to ${config.endDate.slice(0, 7)}` + : null; + const handleGenerate = async () => { if (config.startDate > config.endDate) { - alert('End date must be after start date.'); + // C2: inline error instead of alert() + setErrorMsg('End date must be after start date.'); return; } setGenerating(true); + setErrorMsg(null); try { const reportData = computeReportData(data, config); const blob = await pdf().toBlob(); @@ -44,7 +69,8 @@ export default function ReportPage({ data }: Props) { } } catch (err) { console.error('PDF generation failed:', err); - alert('Failed to generate PDF. Please try again.'); + // C2: inline error instead of alert() + setErrorMsg('Failed to generate PDF. Please try again.'); } finally { setGenerating(false); } @@ -52,28 +78,58 @@ export default function ReportPage({ data }: Props) { return (
+ {/* L2: aria-live region for screen reader status announcements */} +
+ {generating ? 'Generating PDF, please wait.' : ''} + {errorMsg ? `Error: ${errorMsg}` : ''} +
+

Report Builder

-

Configure and download a client-ready PDF report.

+ {/* M5: removed generic filler subtitle */}
-
-
- -
-
+
+ {/* H5: report-footer-chip--count stays visible on mobile; others hide */} + + {sectionCount} section{sectionCount !== 1 ? 's' : ''} + + {periodLabel && ( + <> + {/* L1: aria-hidden on decorative separators */} +
+