From cd9c837f7a5b855423c0d7f3a1f30785ec1269e6 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 8 Mar 2023 20:24:05 -0800 Subject: [PATCH 1/4] Allow ActionList dividers to be added manually --- .../primer/alpha/action_list.html.erb | 5 +-- app/components/primer/alpha/action_list.rb | 23 ++++++++++--- .../primer/alpha/action_list/divider.rb | 4 +-- previews/primer/alpha/action_list_preview.rb | 33 +++++++++++++++++++ test/components/alpha/action_list_test.rb | 26 +++++++++++++++ 5 files changed, 83 insertions(+), 8 deletions(-) diff --git a/app/components/primer/alpha/action_list.html.erb b/app/components/primer/alpha/action_list.html.erb index fe2724dc2a..6bdeee60c5 100644 --- a/app/components/primer/alpha/action_list.html.erb +++ b/app/components/primer/alpha/action_list.html.erb @@ -4,10 +4,11 @@ <% end %> <%= render(Primer::BaseComponent.new(tag: :ul, **@system_arguments)) do %> <% items.each_with_index do |item, index| %> - <% if index > 0 && @show_dividers %> + <%# the conditions here make sure two dividers are never rendered one after the other %> + <% if index > 0 && @show_dividers && !item.is_a?(Divider) && !items[index - 1].is_a?(Divider) %> <%= render(Primer::Alpha::ActionList::Divider.new) %> <% end %> - <%= item %> + <%= render(item) %> <% end %> <% end %> <% end %> diff --git a/app/components/primer/alpha/action_list.rb b/app/components/primer/alpha/action_list.rb index fd409d63a8..210fed0488 100644 --- a/app/components/primer/alpha/action_list.rb +++ b/app/components/primer/alpha/action_list.rb @@ -39,14 +39,28 @@ def custom_element_name Heading.new(list_id: @id, **system_arguments) } - # Items. + # Adds an item to the list. # # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionList::Item) %>. - renders_many :items, lambda { |**system_arguments| - build_item(**system_arguments).tap do |item| + def with_item(**system_arguments) + @items << build_item(**system_arguments).tap do |item| + yield item if block_given? + will_add_item(item) end - } + end + + # Adds a divider to the list of items. + # + # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionList::Divider) %>. + def with_divider(**system_arguments) + @items << ActionList::Divider.new(**system_arguments).tap do |divider| + yield divider if block_given? + end + end + + # Returns the current list of items. + attr_reader :items # @param role [Boolean] ARIA role describing the function of the list. listbox and menu are a common values. # @param item_classes [String] Additional CSS classes to attach to items. @@ -60,6 +74,7 @@ def initialize( show_dividers: false, **system_arguments ) + @items = [] @id = self.class.generate_id @system_arguments = system_arguments diff --git a/app/components/primer/alpha/action_list/divider.rb b/app/components/primer/alpha/action_list/divider.rb index 684cb63acc..4534ccba65 100644 --- a/app/components/primer/alpha/action_list/divider.rb +++ b/app/components/primer/alpha/action_list/divider.rb @@ -16,8 +16,8 @@ class Divider < Primer::Component # @param system_arguments [Hash] <%= link_to_system_arguments_docs %> def initialize(scheme: DEFAULT_SCHEME, **system_arguments) @system_arguments = system_arguments - @system_arguments[:tag] = :div - @system_arguments[:role] = :separator + @system_arguments[:tag] = :li + @system_arguments[:role] = :presentation @system_arguments[:'aria-hidden'] = true @scheme = fetch_or_fallback(SCHEME_OPTIONS, scheme, DEFAULT_SCHEME) @system_arguments[:classes] = class_names( diff --git a/previews/primer/alpha/action_list_preview.rb b/previews/primer/alpha/action_list_preview.rb index 15ec40c926..04ceebbc7b 100644 --- a/previews/primer/alpha/action_list_preview.rb +++ b/previews/primer/alpha/action_list_preview.rb @@ -89,6 +89,39 @@ def leading_visuals( end end + # @label With manual dividers + # + # @param role text + # @param scheme [Symbol] select [full, inset] + # @param show_dividers toggle + def with_manual_dividers( + role: Primer::Alpha::ActionList::DEFAULT_ROLE, + scheme: Primer::Alpha::ActionList::DEFAULT_SCHEME, + show_dividers: false + ) + render(Primer::Alpha::ActionList.new( + role: role, + scheme: scheme, + show_dividers: show_dividers + )) do |component| + component.with_heading(title: "Action List") + component.with_item(label: "Item one", href: "/") do |item| + item.with_leading_visual_icon(icon: :gear) + end + component.with_divider + component.with_item(label: "Item two", href: "/") do |item| + item.with_leading_visual_icon(icon: :star) + end + component.with_item(label: "Item three", href: "/") do |item| + item.with_leading_visual_icon(icon: :heart) + end + component.with_divider + component.with_item(label: "Item four", href: "/") do |item| + item.with_leading_visual_icon(icon: :bug) + end + end + end + # @label Divider # # @param scheme [Symbol] select [subtle, filled] diff --git a/test/components/alpha/action_list_test.rb b/test/components/alpha/action_list_test.rb index 3d7992d4b3..ff17596150 100644 --- a/test/components/alpha/action_list_test.rb +++ b/test/components/alpha/action_list_test.rb @@ -89,6 +89,32 @@ def test_heading_denies_tag_argument assert_match(/This component has a fixed tag/, error.message) end + + def test_manual_dividers + render_inline(Primer::Alpha::ActionList.new(aria: { label: "List" })) do |component| + component.with_item(label: "Item 1", href: "/item1") + component.with_item(label: "Item 2", href: "/item2") + component.with_divider + component.with_item(label: "Item 3", href: "/item3") + end + + list_items = page.find_css("ul.ActionListWrap li").map do |list_item| + classes = list_item["class"].split + + if classes.include?("ActionListItem") + { type: :item, href: list_item.css("a").first["href"] } + elsif classes.include?("ActionList-sectionDivider") + { type: :divider } + end + end + + assert_equal list_items, [ + { type: :item, href: "/item1" }, + { type: :item, href: "/item2" }, + { type: :divider }, + { type: :item, href: "/item3" } + ] + end end end end From 64e45ca1ee02995aec3e50f23b1b06ad4f9c68ff Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 8 Mar 2023 21:25:42 -0800 Subject: [PATCH 2/4] Add changeset --- .changeset/selfish-glasses-count.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/selfish-glasses-count.md diff --git a/.changeset/selfish-glasses-count.md b/.changeset/selfish-glasses-count.md new file mode 100644 index 0000000000..535fe6c653 --- /dev/null +++ b/.changeset/selfish-glasses-count.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Allow ActionList dividers to be added manually From 5636f5f6c55b4ae23a0acaafbaa64416150b4786 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Thu, 9 Mar 2023 13:48:57 -0800 Subject: [PATCH 3/4] Switch back to slot --- .../primer/alpha/action_list.html.erb | 2 +- app/components/primer/alpha/action_list.rb | 22 +++++++------------ 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/app/components/primer/alpha/action_list.html.erb b/app/components/primer/alpha/action_list.html.erb index 6bdeee60c5..d65e86ffdb 100644 --- a/app/components/primer/alpha/action_list.html.erb +++ b/app/components/primer/alpha/action_list.html.erb @@ -8,7 +8,7 @@ <% if index > 0 && @show_dividers && !item.is_a?(Divider) && !items[index - 1].is_a?(Divider) %> <%= render(Primer::Alpha::ActionList::Divider.new) %> <% end %> - <%= render(item) %> + <%= item %> <% end %> <% end %> <% end %> diff --git a/app/components/primer/alpha/action_list.rb b/app/components/primer/alpha/action_list.rb index 210fed0488..c99494bdeb 100644 --- a/app/components/primer/alpha/action_list.rb +++ b/app/components/primer/alpha/action_list.rb @@ -39,29 +39,24 @@ def custom_element_name Heading.new(list_id: @id, **system_arguments) } - # Adds an item to the list. + # Items. # # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionList::Item) %>. - def with_item(**system_arguments) - @items << build_item(**system_arguments).tap do |item| - yield item if block_given? - + renders_many :items, lambda { |**system_arguments| + build_item(**system_arguments).tap do |item| will_add_item(item) end - end + } # Adds a divider to the list of items. # # @param system_arguments [Hash] The arguments accepted by <%= link_to_component(Primer::Alpha::ActionList::Divider) %>. - def with_divider(**system_arguments) - @items << ActionList::Divider.new(**system_arguments).tap do |divider| - yield divider if block_given? - end + def with_divider(**system_arguments, &block) + # This is a giant hack that should be removed when :items can be converted into a polymorphic slot. + # This feature needs to land in view_component first: https://github.com/ViewComponent/view_component/pull/1652 + set_slot(:items, { renderable: Divider, collection: true }, **system_arguments, &block) end - # Returns the current list of items. - attr_reader :items - # @param role [Boolean] ARIA role describing the function of the list. listbox and menu are a common values. # @param item_classes [String] Additional CSS classes to attach to items. # @param scheme [Symbol] <%= one_of(Primer::Alpha::ActionList::SCHEME_OPTIONS) %>. `inset` children are offset (vertically and horizontally) from list edges. `full` (default) children are flush (vertically and horizontally) with list edges. @@ -74,7 +69,6 @@ def initialize( show_dividers: false, **system_arguments ) - @items = [] @id = self.class.generate_id @system_arguments = system_arguments From 530218b121fab77ec7d0c36dede1ae8f070adbc0 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Thu, 9 Mar 2023 14:36:59 -0800 Subject: [PATCH 4/4] Allow aria-labelledby instead of aria-label --- app/components/primer/alpha/action_list.rb | 5 +++-- test/components/alpha/action_list_test.rb | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/components/primer/alpha/action_list.rb b/app/components/primer/alpha/action_list.rb index c99494bdeb..eca08f52f1 100644 --- a/app/components/primer/alpha/action_list.rb +++ b/app/components/primer/alpha/action_list.rb @@ -90,12 +90,13 @@ def initialize( # @private def before_render aria_label = aria(:label, @system_arguments) + aria_labelledby = aria(:labelledby, @system_arguments) if heading.present? @system_arguments[:"aria-labelledby"] = @id raise ArgumentError, "An aria-label should not be provided if a heading is present" if aria_label.present? - elsif aria_label.blank? - raise ArgumentError, "An aria-label or heading must be provided" + elsif aria_label.blank? && aria_labelledby.blank? + raise ArgumentError, "An aria-label, aria-labelledby, or heading must be provided" end end diff --git a/test/components/alpha/action_list_test.rb b/test/components/alpha/action_list_test.rb index 60d9aa7409..a9d31a3057 100644 --- a/test/components/alpha/action_list_test.rb +++ b/test/components/alpha/action_list_test.rb @@ -12,7 +12,7 @@ def test_invalid_list render_inline(Primer::Alpha::ActionList.new) end - assert_includes(error.message, "label or heading must be provided") + assert_includes(error.message, "aria-label, aria-labelledby, or heading must be provided") end def test_active_item