fix(ds): migrate goals color picker back to DS::Disclosure (#2350)

Commit 5d0eb7f4 replaced the color picker's DS::Disclosure with a raw
<details> 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
<details> 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").
This commit is contained in:
Guillem Arias Fauste
2026-06-15 20:13:15 +02:00
committed by GitHub
parent 2d3d974466
commit beec50b4a4
4 changed files with 52 additions and 14 deletions

View File

@@ -33,7 +33,7 @@
<% end %>
<% end %>
<div class="mt-2">
<%= tag.div class: body_class.presence do %>
<%= content %>
</div>
<% end %>
<% end %>

View File

@@ -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 `<details>`. 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)

View File

@@ -14,16 +14,20 @@
<% end %>
</span>
<%# Raw <details> 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. %>
<details class="group"
data-color-icon-picker-target="details"
data-action="mousedown->color-icon-picker#handleOutsideClick">
<summary aria-label="<%= t("goals.color_picker.trigger_label") %>"
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 %>
<span class="sr-only"><%= t("goals.color_picker.trigger_label") %></span>
<%= icon("pen", size: "xs") %>
</summary>
<% end %>
<div class="absolute top-full left-1/2 -translate-x-1/2 mt-2 z-50 bg-container p-3 border border-alpha-black-25 rounded-2xl shadow-xs w-80 max-w-[calc(100vw-2rem)] max-h-[60vh] overflow-y-auto"
data-color-icon-picker-target="popup">
@@ -69,6 +73,6 @@
</div>
</div>
</div>
</details>
<% end %>
</div>
</div>

View File

@@ -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