From 1b482563e1ff30ad5ae0b4437057f5d2532ff59b Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 30 Sep 2022 15:37:07 -0700 Subject: [PATCH 1/2] Allow NavList items to be selected by the current page --- .../primer/alpha/action_list/item.rb | 8 +-- app/components/primer/alpha/nav_list/item.rb | 50 ++++++++++++++++--- .../primer/alpha/nav_list/section.rb | 5 -- test/components/alpha/nav_list_test.rb | 28 +++++++++++ 4 files changed, 74 insertions(+), 17 deletions(-) diff --git a/app/components/primer/alpha/action_list/item.rb b/app/components/primer/alpha/action_list/item.rb index eab3f1430d..a29eaf1f14 100644 --- a/app/components/primer/alpha/action_list/item.rb +++ b/app/components/primer/alpha/action_list/item.rb @@ -112,7 +112,7 @@ class Item < Primer::Component # @private renders_one :private_content - attr_reader :list, :active, :disabled, :parent + attr_reader :list, :href, :active, :disabled, :parent # Whether or not this item is active. # @@ -176,8 +176,7 @@ def initialize( @system_arguments[:classes] = class_names( @system_arguments[:classes], SCHEME_MAPPINGS[@scheme], - "ActionListItem", - "ActionListItem--navActive" => @active + "ActionListItem" ) @system_arguments[:role] = role @@ -223,7 +222,8 @@ def before_render @system_arguments[:classes] = class_names( @system_arguments[:classes], "ActionListItem--withActions" => trailing_action.present?, - "ActionListItem--trailingActionHover" => @trailing_action_on_hover + "ActionListItem--trailingActionHover" => @trailing_action_on_hover, + "ActionListItem--navActive" => active? ) return unless leading_visual diff --git a/app/components/primer/alpha/nav_list/item.rb b/app/components/primer/alpha/nav_list/item.rb index c9d9b3bc92..83f910026c 100644 --- a/app/components/primer/alpha/nav_list/item.rb +++ b/app/components/primer/alpha/nav_list/item.rb @@ -16,13 +16,6 @@ class Item < Primer::Alpha::ActionList::Item @list.build_item(parent: self, sub_item: true, **system_arguments).tap do |item| @list.will_add_item(item) - - if item.active? - @content_arguments[:classes] = class_names( - @content_arguments[:classes], - "ActionListContent--hasActiveSubItem" - ) - end end } @@ -55,17 +48,29 @@ def initialize(selected_item_id: nil, selected_by_ids: [], sub_item: false, expa } overrides = { "data-item-id": @selected_by_ids.join(" ") } - overrides[:active] = @selected_by_ids.include?(@selected_item_id) super(**system_arguments, **overrides) end + def active? + item_active?(self) + end + # Cause this item to show its list of sub items when rendered. def expand! @expanded = true end def before_render + if active_sub_item? + expand! + + @content_arguments[:classes] = class_names( + @content_arguments[:classes], + "ActionListContent--hasActiveSubItem" + ) + end + super raise "Cannot render a trailing visual for an item with subitems" if items.present? && trailing_visual.present? @@ -83,6 +88,35 @@ def before_render "ActionListItem--hasSubItem" ) end + + private + + # Normally it would be easier to simply ask each item for its active status, eg. + # items.any?(&:active?), but unfortunately the view context is not set on each + # item until _after_ the parent's before_render, etc methods have been called. + # This means helper methods like current_page? will blow up with an error, since + # `helpers` is simply an alias for the view context (i.e. an instance of + # ActionView::Base). Since we know the view context for the parent object must + # be set before `before_render` is invoked, we can call helper methods here in + # the parent and bypass the problem entirely. Maybe not the most OO approach, + # but it works. + def item_active?(item) + if item.selected_by_ids.present? + item.selected_by_ids.include?(@selected_item_id) + elsif item.href + current_page?(item.href) + else + false + end + end + + def active_sub_item? + items.any? { |subitem| item_active?(subitem) } + end + + def current_page?(url) + helpers.current_page?(url) + end end end end diff --git a/app/components/primer/alpha/nav_list/section.rb b/app/components/primer/alpha/nav_list/section.rb index 3a5c824750..53381ed768 100644 --- a/app/components/primer/alpha/nav_list/section.rb +++ b/app/components/primer/alpha/nav_list/section.rb @@ -77,11 +77,6 @@ def build_item(component_klass: NavList::Item, **system_arguments) list: self ) end - - # @private - def will_add_item(item) - item.parent.expand! if item.active? && item.parent - end end end end diff --git a/test/components/alpha/nav_list_test.rb b/test/components/alpha/nav_list_test.rb index b83a5db726..db9f0a2c20 100644 --- a/test/components/alpha/nav_list_test.rb +++ b/test/components/alpha/nav_list_test.rb @@ -64,6 +64,34 @@ def test_item_can_be_selected_by_any_of_its_ids refute_selector ".ActionListItem--navActive", text: "Item 2" end + class CurrentPageItem < Primer::Alpha::NavList::Item + def initialize(current_page_href:, **system_arguments) + @current_page_href = current_page_href + super(**system_arguments) + end + + private + + def current_page?(url) + url == @current_page_href + end + end + + def test_item_can_be_selected_by_current_page + current_page_href = "/item2" + + render_inline(Primer::Alpha::NavList.new) do |c| + c.with_section(aria: { label: "Nav list" }) do |section| + # use CurrentPageItem instead of a mock since the API supports it + section.with_item(component_klass: CurrentPageItem, label: "Item 1", href: "/item1", current_page_href: current_page_href) + section.with_item(component_klass: CurrentPageItem, label: "Item 2", href: "/item2", current_page_href: current_page_href) + end + end + + refute_selector ".ActionListItem--navActive", text: "Item 1" + assert_selector ".ActionListItem--navActive", text: "Item 2" + end + def test_max_nesting_depth error = assert_raises(RuntimeError) do render_inline(Primer::Alpha::NavList.new) do |c| From 6ff9c9236f8ce01ff399ff13985c759f8574eaf9 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Fri, 30 Sep 2022 15:39:41 -0700 Subject: [PATCH 2/2] Add changeset --- .changeset/twelve-doors-tease.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/twelve-doors-tease.md diff --git a/.changeset/twelve-doors-tease.md b/.changeset/twelve-doors-tease.md new file mode 100644 index 0000000000..3c7455a5ec --- /dev/null +++ b/.changeset/twelve-doors-tease.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Allow NavList items to be selected by the current page