From f50c151e21d1db2a5808f0698e794b544dfff0f4 Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Mon, 11 May 2026 23:29:05 +0200 Subject: [PATCH] fix(design-system): DS::Alert alignment, accessibility, and hierarchy polish (#1734) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(design-system): align DS::Alert icon with title The icon was rendered at size 'sm' (w-4 h-4) and started at the very top of the flex row (items-start without an offset), which optically sat above the title's cap when the title was present and slightly above the message baseline when it wasn't. The hand-rolled alerts this PR replaced used 'w-5 h-5 mt-0.5' for exactly this reason — restore the same combination in the component: - size: sm -> md (w-4/h-4 -> w-5/h-5). - class adds mt-0.5 so the icon's vertical center lines up with the bold title's cap-height (and with the body baseline in the title-less case). No API change. Visual fix only. Refs #1731 * fix(design-system): split DS::Alert into title-row + indented body Replaces the items-start + margin-fudge approach with a two-row layout that doesn't depend on icon-bounding-box vs text-cap-height arithmetic: - Title case: icon and bold title share a flex row with items-center, so the icon's vertical centre lines up with the title's line. Body (block content or message) renders below in a separate row, padded by pl-8 (= icon md width + gap-3) so it indents under the title text rather than under the icon. - Block-only case (no title, no message — used by the alpha_vantage rate-limit alert): keeps the items-start fallback with a small mt-0.5 on the icon so the cap of the first paragraph still sits near the icon centre. - Single-line message case: items-center between icon and message, no fudge needed. container_classes loses its 'flex items-start gap-3' base since the outer div is no longer the flex container. Each branch declares its own flex/items-* combination. Refs #1731 * fix(design-system): a11y semantics + visual polish on DS::Alert Builds on the title-row restructure with the items the design / a11y review surfaced: - live: keyword (default :none, accepts :status / :polite and :alert / :assertive) maps to role="status" or role="alert" on the outer div. Static, page-baked alerts (the migrated callsites in #1731) keep the default :none and stay role-less. Dynamic surfaces (flash, validation summaries appearing after a Turbo update) opt into the live role they need. - aria-labelledby on the outer div pointing at the title

so AT picks the title as the alert's accessible name when one is set. - Variant prefix in the title / message via an sr-only span. Screen reader hears 'Warning: …', 'Error: …', etc.; sighted users see no change. Variant labels live under ds.alert.variants.* in config/locales/views/components/en.yml. - Body text inside titled alerts now defaults to text-secondary instead of text-primary, so hierarchy reads on weight + colour rather than weight alone (Refactoring UI: hierarchy needs both). Single-line message and block-only fallback keep text-primary since there is no second tier. - Icon size goes back from md (20px) to sm (16px) — proportionally closer to text-sm body — and the items-center branches grow -mt-0.5 to compensate for the cap-centre vs line-centre offset that flex's items-center alone can't bridge. - Title weight bumped from font-medium (500) to font-semibold (600) for clearer prominence against the now-softer body. No API breakage: existing callers passing only message:/title:/variant: keep working. The new live: arg defaults to the correct value for the static migration sites. Refs #1731 * fix(design-system): drop aria-labelledby when alert has no role; revert body to text-primary Two corrections after numerical contrast analysis and CodeRabbit feedback: 1. aria-labelledby was being emitted on every titled alert, but the default live: :none leaves the outer

with no role. ARIA spec only honours the labelling relationship on elements with a host role, so on a generic
the attribute is invalid and accessibility validators flag it. Now only emitted when aria_role is set (live: :status or :alert). Static, page-baked callsites stay role-less and label-less; dynamic callers that opt into a live role get the proper accessible-name relationship. 2. text-secondary on bg-{variant}/10 in light mode lands at ~4.07-4.25:1 contrast — below WCAG AA's 4.5:1 for normal text. Reverting the body wrapper to text-primary brings it back to AAA (~15:1). Loses some of the Refactoring UI body-vs-title colour hierarchy; the title's font-semibold weight + larger optical mass against an otherwise plain body still reads as hierarchy. Single-line message and block-only fallback already used text-primary, so this just unifies the three branches. The remaining contrast gap — text-success (green-600) icon on bg-success/10 light surface at 2.77:1 — is documented in the PR description; fixing it cleanly needs a token-level bump (--color-success: green-600 -> green-700 in light mode) which is out of scope for this PR. Refs #1731 * fix(settings/providers): use DS::Alert title:+message: instead of inline content_tag Three callsites added in #1710 passed block-level markup (`

`/`

`) through `message:` via `safe_join + content_tag`. The post-#1731 alert template wraps `message:` in a `

`, which makes nesting a `

` or `

` invalid HTML — browsers auto-close the outer paragraph and the indented body row collapses. Each of the three is semantically a title + body pair, so swap them to the proper `title:` + `message:` API. No new strings — the i18n keys (`*.no_withdraw_title` / `_body`, `encryption_error.title` / `.message`) already split that way; the inline assembly was the artefact. The encryption-error block loses an explicit `

` wrapper around the title; DS::Alert's title is a `

`. The visual hierarchy and sr-only variant prefix are unchanged. Worth tracking heading semantics as a follow-up against DS::Alert (a `heading_level:` arg) rather than bringing back the manual markup. * fix(design-system): make :destructive variant alias explicit in DS::Alert locale Add `destructive: Error` to `ds.alert.variants` and drop the implicit `:destructive -> :error` aliasing in `DS::Alert#variant_label`. Both the locale file and the component now self-document the variant set; lookup is direct, no conditional needed. Per @jjmata review on #1734. --- app/components/DS/alert.html.erb | 48 ++++++++++++++----- app/components/DS/alert.rb | 33 +++++++++++-- .../providers/_binance_panel.html.erb | 6 +-- .../providers/_enable_banking_panel.html.erb | 6 +-- app/views/settings/providers/show.html.erb | 6 +-- config/locales/views/components/en.yml | 8 ++++ 6 files changed, 79 insertions(+), 28 deletions(-) diff --git a/app/components/DS/alert.html.erb b/app/components/DS/alert.html.erb index 419a4607c..efda08221 100644 --- a/app/components/DS/alert.html.erb +++ b/app/components/DS/alert.html.erb @@ -1,15 +1,37 @@ -

- <%= helpers.icon icon_name, size: "sm", color: icon_color, class: "shrink-0 mt-0.5" %> +<%= tag.div(class: container_classes, role: aria_role, "aria-labelledby": (aria_role && title.present?) ? title_id : nil) do %> + <% if title.present? %> +
+ <%= helpers.icon icon_name, size: "sm", color: icon_color, class: "shrink-0 -mt-0.5" %> +

+ <%= variant_label %>: + <%= title %> +

+
-
- <% if title.present? %> -

<%= title %>

+ <% if content.present? || message.present? %> +
+ <% if content.present? %> + <%= content %> + <% else %> + <%= message %> + <% end %> +
<% end %> - - <% if content.present? %> - <%= content %> - <% elsif message.present? %> - <%= message %> - <% end %> -
-
+ <% elsif content.present? %> +
+ <%= helpers.icon icon_name, size: "sm", color: icon_color, class: "shrink-0" %> +
+ <%= variant_label %>: + <%= content %> +
+
+ <% elsif message.present? %> +
+ <%= helpers.icon icon_name, size: "sm", color: icon_color, class: "shrink-0 -mt-0.5" %> +

+ <%= variant_label %>: + <%= message %> +

+
+ <% end %> +<% end %> diff --git a/app/components/DS/alert.rb b/app/components/DS/alert.rb index dc7025eb7..050beaf66 100644 --- a/app/components/DS/alert.rb +++ b/app/components/DS/alert.rb @@ -1,22 +1,34 @@ class DS::Alert < DesignSystemComponent VARIANTS = %i[info success warning error destructive].freeze + LIVE_MODES = %i[none status alert].freeze - def initialize(message: nil, title: nil, variant: :info) + def initialize(message: nil, title: nil, variant: :info, live: :none) @message = message @title = title @variant = normalize_variant(variant) + @live = normalize_live(live) end private - attr_reader :message, :title, :variant + attr_reader :message, :title, :variant, :live def normalize_variant(raw) sym = raw.respond_to?(:to_sym) ? raw.to_sym : nil VARIANTS.include?(sym) ? sym : :info end + def normalize_live(raw) + sym = raw.respond_to?(:to_sym) ? raw.to_sym : nil + case sym + when :polite then :status + when :assertive then :alert + when *LIVE_MODES then sym + else :none + end + end + def container_classes - base_classes = "flex items-start gap-3 p-4 rounded-lg border" + base_classes = "p-4 rounded-lg border" variant_classes = case variant when :info @@ -57,4 +69,19 @@ class DS::Alert < DesignSystemComponent "info" end end + + def aria_role + case live + when :status then "status" + when :alert then "alert" + end + end + + def variant_label + I18n.t("ds.alert.variants.#{variant}") + end + + def title_id + @title_id ||= "DS-alert-title-#{SecureRandom.hex(4)}" + end end diff --git a/app/views/settings/providers/_binance_panel.html.erb b/app/views/settings/providers/_binance_panel.html.erb index f93eeea47..49b8cd0d4 100644 --- a/app/views/settings/providers/_binance_panel.html.erb +++ b/app/views/settings/providers/_binance_panel.html.erb @@ -3,10 +3,8 @@ <%= render DS::Alert.new( variant: :warning, - message: safe_join([ - content_tag(:p, t("settings.providers.binance_panel.no_withdraw_title"), class: "font-medium"), - content_tag(:p, t("settings.providers.binance_panel.no_withdraw_body"), class: "mt-1") - ]) + title: t("settings.providers.binance_panel.no_withdraw_title"), + message: t("settings.providers.binance_panel.no_withdraw_body") ) %>
diff --git a/app/views/settings/providers/_enable_banking_panel.html.erb b/app/views/settings/providers/_enable_banking_panel.html.erb index 4085520ef..45344ea55 100644 --- a/app/views/settings/providers/_enable_banking_panel.html.erb +++ b/app/views/settings/providers/_enable_banking_panel.html.erb @@ -71,10 +71,8 @@ <% if has_authenticated_connections && !is_new_record %> <%= render DS::Alert.new( variant: :warning, - message: safe_join([ - content_tag(:p, "Configuration locked", class: "font-medium"), - content_tag(:p, "Disconnect all linked banks before changing these credentials.", class: "mt-1") - ]) + title: "Configuration locked", + message: "Disconnect all linked banks before changing these credentials." ) %> <% end %> diff --git a/app/views/settings/providers/show.html.erb b/app/views/settings/providers/show.html.erb index b78a19a96..9108eabcd 100644 --- a/app/views/settings/providers/show.html.erb +++ b/app/views/settings/providers/show.html.erb @@ -4,10 +4,8 @@ <% if @encryption_error %> <%= render DS::Alert.new( variant: :error, - message: safe_join([ - content_tag(:h2, t("settings.providers.encryption_error.title"), class: "font-medium"), - content_tag(:p, t("settings.providers.encryption_error.message"), class: "text-sm mt-1") - ]) + title: t("settings.providers.encryption_error.title"), + message: t("settings.providers.encryption_error.message") ) %> <% else %>
diff --git a/config/locales/views/components/en.yml b/config/locales/views/components/en.yml index 9230a79ec..b17124e37 100644 --- a/config/locales/views/components/en.yml +++ b/config/locales/views/components/en.yml @@ -1,5 +1,13 @@ --- en: + ds: + alert: + variants: + info: Info + success: Success + warning: Warning + error: Error + destructive: Error provider_sync_summary: title: Sync summary last_sync: "Last sync: %{time_ago} ago"