From db563f3d62306f7c632307e2e7aa7227e44fc2d6 Mon Sep 17 00:00:00 2001 From: Guillem Arias Date: Tue, 19 May 2026 16:26:08 +0200 Subject: [PATCH] fix(review): href-branch type-button bug + focus-ring tokens + profile Save submit CodeRabbit P1+P2 review on #1840: 1. button.rb: `merged_opts.delete(:href)` always returned nil because Buttonish#initialize strips :href from opts into @href, so the `if href.blank?` guard was ALWAYS true. Every DS::Button rendered via button_to (the href branch) got `type="button"` on the inner button, breaking submission of those button_to-generated forms (e.g. imports/_ready.html.erb publish button, imports/_failure.html.erb try-again button). Drop the local `href = merged_opts.delete(:href)` so the guard now reads the @href reader, leaving the href branch's HTML default intact. 2. settings/profiles/show.html.erb: the Save button is rendered with `render DS::Button.new(...)` inside `styled_form_with` (not via form.submit), so the StyledFormBuilder#submit type-pin from 624e9794 doesn't cover it. Pass `type: :submit` explicitly so the profile form submits again under the default-type-button policy. 3. base.css: replace raw `outline-gray-900` / `outline-white` with the established alpha-ring focus pattern (focus-visible:ring-alpha-black-300 + theme-dark:ring-alpha-white-300) already used by app/components/settings/provider_card.html.erb and sure-design-system/components.css. Keeps a11y focus ring while using DS tokens. --- app/assets/tailwind/sure-design-system/base.css | 4 ++-- app/components/DS/button.rb | 4 ++-- app/views/settings/profiles/show.html.erb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/assets/tailwind/sure-design-system/base.css b/app/assets/tailwind/sure-design-system/base.css index 10a6bbd33..522cdcf37 100644 --- a/app/assets/tailwind/sure-design-system/base.css +++ b/app/assets/tailwind/sure-design-system/base.css @@ -1,9 +1,9 @@ @layer base { button { - @apply cursor-pointer focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-gray-900; + @apply cursor-pointer focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-alpha-black-300; @variant theme-dark { - @apply focus-visible:outline-white; + @apply focus-visible:ring-alpha-white-300; } } diff --git a/app/components/DS/button.rb b/app/components/DS/button.rb index 667b85e3d..ca5644225 100644 --- a/app/components/DS/button.rb +++ b/app/components/DS/button.rb @@ -22,7 +22,6 @@ class DS::Button < DS::Buttonish def merged_opts merged_opts = opts.dup || {} extra_classes = merged_opts.delete(:class) - href = merged_opts.delete(:href) data = merged_opts.delete(:data) || {} if confirm.present? @@ -37,7 +36,8 @@ class DS::Button < DS::Buttonish # spec — meaning a DS::Button rendered inside a form will steal Enter-key # submission from the first text input. Default to `type="button"` so # callers must opt into submit behavior explicitly. `button_to` (href - # branch) wraps the button in its own form, so submit there is correct. + # branch) wraps the button in its own form, so submit there is correct + # and we leave its default alone. if href.blank? merged_opts[:type] ||= "button" end diff --git a/app/views/settings/profiles/show.html.erb b/app/views/settings/profiles/show.html.erb index 245ac27db..ebe10ec92 100644 --- a/app/views/settings/profiles/show.html.erb +++ b/app/views/settings/profiles/show.html.erb @@ -19,7 +19,7 @@
- <%= render DS::Button.new(text: t(".save"), class: "md:w-auto w-full justify-center") %> + <%= render DS::Button.new(text: t(".save"), type: :submit, class: "md:w-auto w-full justify-center") %>
<% end %>