From beec50b4a4636e53b1dd4d1de5752ae1745e8a36 Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Mon, 15 Jun 2026 20:13:15 +0200 Subject: [PATCH] fix(ds): migrate goals color picker back to DS::Disclosure (#2350) Commit 5d0eb7f4 replaced the color picker's DS::Disclosure with a raw
because DS::Disclosure wraps its body in an mt-2 div, and that normal-flow margin shoved the form down ~8px whenever the absolutely- positioned picker popover opened. DS Drift Patrol flagged the raw
in #2272 and #2316. Add a body_class: option to DS::Disclosure (default mt-2) so callers can drop the body margin, and migrate the color picker back onto the component with body_class: nil. The summary's accessible name moves from aria-label to an sr-only span in the summary content (verified: the summary still reads as "Choose color and icon"). --- app/components/DS/disclosure.html.erb | 4 ++-- app/components/DS/disclosure.rb | 10 +++++++-- app/views/goals/_color_picker.html.erb | 24 +++++++++++++--------- test/components/DS/disclosure_test.rb | 28 ++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 14 deletions(-) create mode 100644 test/components/DS/disclosure_test.rb diff --git a/app/components/DS/disclosure.html.erb b/app/components/DS/disclosure.html.erb index c323aa76c..8cd25bcfc 100644 --- a/app/components/DS/disclosure.html.erb +++ b/app/components/DS/disclosure.html.erb @@ -33,7 +33,7 @@ <% end %> <% end %> -
+ <%= tag.div class: body_class.presence do %> <%= content %> -
+ <% end %> <% end %> diff --git a/app/components/DS/disclosure.rb b/app/components/DS/disclosure.rb index b32433a6f..bff18f332 100644 --- a/app/components/DS/disclosure.rb +++ b/app/components/DS/disclosure.rb @@ -3,7 +3,7 @@ class DS::Disclosure < DesignSystemComponent VARIANTS = %i[default card card_inset inline].freeze - attr_reader :title, :align, :open, :variant, :summary_class_override, :opts + attr_reader :title, :align, :open, :variant, :summary_class_override, :body_class, :opts # `:default` — bg-surface summary, no chrome on the `
`. Use # for inline expanders that sit inside a parent card (the summary @@ -28,12 +28,18 @@ class DS::Disclosure < DesignSystemComponent # In card / inline variants, callers should pass their own # `summary_content` slot; the built-in title rendering assumes the # `:default` shape. - def initialize(title: nil, align: "right", open: false, variant: :default, summary_class: nil, **opts) + # `body_class:` styles the wrapper around the disclosure body. Defaults + # to `mt-2` (the standard gap below the summary). Pass `nil`/`""` to drop + # it — e.g. when the body is an absolutely-positioned popover whose + # wrapper would otherwise add ~8px of normal-flow margin and shove + # siblings down on open (see `goals/_color_picker`). + def initialize(title: nil, align: "right", open: false, variant: :default, summary_class: nil, body_class: "mt-2", **opts) @title = title @align = align.to_sym @open = open @variant = variant&.to_sym @summary_class_override = summary_class + @body_class = body_class @opts = opts raise ArgumentError, "Invalid variant: #{@variant.inspect}. Must be one of #{VARIANTS.inspect}" unless VARIANTS.include?(@variant) diff --git a/app/views/goals/_color_picker.html.erb b/app/views/goals/_color_picker.html.erb index dc5292222..784091394 100644 --- a/app/views/goals/_color_picker.html.erb +++ b/app/views/goals/_color_picker.html.erb @@ -14,16 +14,20 @@ <% end %> - <%# Raw
instead of DS::Disclosure: the disclosure wraps its body - in an `mt-2` div that lives in normal flow, which pushed the form down - ~8px every time this absolutely-positioned popover opened. %> -
- " - class="list-none cursor-pointer absolute -bottom-1 -right-1 flex justify-center items-center bg-surface-inset hover:bg-surface-inset-hover border-2 w-6 h-6 border-subdued rounded-full text-secondary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-alpha-black-300"> + <%# `body_class: nil` drops DS::Disclosure's default `mt-2` body wrapper: + the body here is an absolutely-positioned popover, so that wrapper's + normal-flow margin would otherwise push the form down ~8px on open. %> + <%= render DS::Disclosure.new( + body_class: nil, + summary_class: "list-none cursor-pointer absolute -bottom-1 -right-1 flex justify-center items-center bg-surface-inset hover:bg-surface-inset-hover border-2 w-6 h-6 border-subdued rounded-full text-secondary focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-alpha-black-300", + data: { + color_icon_picker_target: "details", + action: "mousedown->color-icon-picker#handleOutsideClick" + }) do |disclosure| %> + <% disclosure.with_summary_content do %> + <%= t("goals.color_picker.trigger_label") %> <%= icon("pen", size: "xs") %> - + <% end %>
@@ -69,6 +73,6 @@
-
+ <% end %> diff --git a/test/components/DS/disclosure_test.rb b/test/components/DS/disclosure_test.rb new file mode 100644 index 000000000..b8154a49f --- /dev/null +++ b/test/components/DS/disclosure_test.rb @@ -0,0 +1,28 @@ +require "test_helper" + +class DS::DisclosureTest < ViewComponent::TestCase + test "body wrapper defaults to an mt-2 margin" do + render_inline(DS::Disclosure.new(title: "More", open: true)) { "body text" } + + assert_selector "details > div.mt-2", text: "body text" + end + + test "body_class: nil drops the body margin wrapper" do + render_inline(DS::Disclosure.new(title: "More", open: true, body_class: nil)) { "body text" } + + assert_no_selector "details > div.mt-2" + assert_selector "details > div", text: "body text" + end + + test "forwards data attributes and a summary_class override" do + render_inline(DS::Disclosure.new( + summary_class: "custom-summary", + data: { controller: "color-icon-picker", action: "mousedown->color-icon-picker#handleOutsideClick" } + )) do |disclosure| + disclosure.with_summary_content { "trigger" } + end + + assert_selector "details[data-controller='color-icon-picker']" + assert_selector "summary.custom-summary", text: "trigger" + end +end