Skip to content

Commit cf4723e

Browse files
committed
* Fix Bug where single sections was not preserved after update
* Add tests for TrailingActionButton * Fix eslint issues * Include review feedback
1 parent 0faf728 commit cf4723e

14 files changed

Lines changed: 104 additions & 39 deletions

File tree

.changeset/afraid-poets-sing.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
'@openproject/primer-view-components': minor
33
---
44

5-
Async loading strategy for FitlerableTreeView
5+
Async loading strategy for FilterableTreeView

.changeset/loud-paths-taste.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
'@openproject/primer-view-components': minor
33
---
44

5-
Add trailingActionIcon slot to TreeView and FilterableTreeView
5+
Add trailing action button support to TreeView and FilterableTreeView

app/components/primer/open_project/filterable_tree_view.html.erb

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
<%= render(Primer::Alpha::Stack.new(wrap: :wrap, direction: :horizontal, align: :center)) do %>
44
<%= render(Primer::Alpha::StackItem.new(grow: true)) do %>
55
<%= render(@filter_input) do |input| %>
6-
<%# exclude from form submissions %>
76
<% input.merge_input_arguments!(form: "") %>
87
<% end %>
98
<% end %>

app/components/primer/open_project/filterable_tree_view.pcss

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,29 +29,28 @@ filterable-tree-view {
2929
}
3030

3131
/* Highlight style for CSS Custom Highlight API */
32-
/* stylelint-disable-next-line selector-pseudo-element-no-unknown */
3332
::highlight(primer-filterable-tree-view-search-results) {
3433
background-color: var(--bgColor-attention-muted);
3534
}
3635

3736
/* Fallback: <mark> elements used when CSS Custom Highlight API is unavailable */
37+
/* stylelint-disable-next-line selector-max-type */
3838
filterable-tree-view mark {
3939
background-color: var(--bgColor-attention-muted);
4040
color: inherit;
4141
}
4242

4343
/* Loading skeleton */
44-
4544
.FilterableTreeViewLoadingSkeleton {
4645
display: none;
4746
}
4847

49-
/* stylelint-disable-next-line selector-max-type */
48+
/* stylelint-disable selector-no-qualifying-type */
49+
/* stylelint-disable selector-max-type */
5050
filterable-tree-view[data-loading] .FilterableTreeViewLoadingSkeleton {
5151
display: block;
5252
}
5353

54-
/* stylelint-disable-next-line selector-max-type */
5554
filterable-tree-view[data-loading] tree-view,
5655
filterable-tree-view[data-loading] [data-target~="filterable-tree-view.noResultsMessage"] {
5756
display: none;

app/components/primer/open_project/filterable_tree_view.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@ module OpenProject
138138
class FilterableTreeView < Primer::Component
139139
delegate :with_leaf, :with_sub_tree, to: :@tree_view
140140

141+
SUPPORTED_SELECT_VARIANTS = %i[multiple single none].freeze
142+
141143
DEFAULT_FILTER_INPUT_ARGUMENTS = {
142144
name: :filter,
143145
label: I18n.t(:button_filter),
@@ -267,13 +269,11 @@ def with_filter_mode(name:, **system_arguments)
267269
@filter_mode_control.with_item(**system_arguments)
268270
end
269271

270-
SUPPORTED_SELECT_VARIANTS = %i[multiple single none].freeze
271-
272272
def with_sub_tree(**system_arguments, &block)
273273
system_arguments[:select_variant] ||= :multiple
274274

275275
unless SUPPORTED_SELECT_VARIANTS.include?(system_arguments[:select_variant])
276-
raise ArgumentError, "FilterableTreeView only supports #{SUPPORTED_SELECT_VARIANTS.map { |v| "`:#{v}`" }.join(", ")} as select_variant"
276+
raise ArgumentError, "FilterableTreeView only supports #{SUPPORTED_SELECT_VARIANTS.map(&:inspect).to_sentence} as select_variant"
277277
end
278278

279279
if system_arguments[:select_variant] != :multiple
@@ -294,7 +294,7 @@ def with_leaf(**system_arguments, &block)
294294
system_arguments[:select_variant] ||= :multiple
295295

296296
unless SUPPORTED_SELECT_VARIANTS.include?(system_arguments[:select_variant])
297-
raise ArgumentError, "FilterableTreeView only supports #{SUPPORTED_SELECT_VARIANTS.map { |v| "`:#{v}`" }.join(", ")} as select_variant"
297+
raise ArgumentError, "FilterableTreeView only supports #{SUPPORTED_SELECT_VARIANTS.map(&:inspect).to_sentence} as select_variant"
298298
end
299299

300300
if system_arguments[:select_variant] != :multiple

app/components/primer/open_project/filterable_tree_view.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import {controller, target} from '@github/catalyst'
22
import {SegmentedControlElement} from '../alpha/segmented_control'
33
import {TreeViewElement} from '../alpha/tree_view/tree_view'
44
import {TreeViewSubTreeNodeElement} from '../alpha/tree_view/tree_view_sub_tree_node_element'
5+
// eslint-disable-next-line import/named
56
import {TreeViewCheckedValue, TreeViewNodeInfo} from '../shared_events'
67

78
// This function is expected to return the following values:
@@ -108,6 +109,15 @@ export class FilterableTreeViewElement extends HTMLElement {
108109
this.#checkedNodeIds.delete(nodeId)
109110
this.#checkedNodeFormPayloads.delete(nodeId)
110111
} else {
112+
// In single-select mode, TreeView clears the previous selection internally
113+
// (via checkOnlyAtPath) but the treeViewNodeChecked event only contains the
114+
// newly selected node. Clear our tracked state so #restoreSelectionState does
115+
// not re-check previously selected nodes after a tree replacement.
116+
if (node.getAttribute('data-select-variant') === 'single') {
117+
this.#checkedNodeIds.clear()
118+
this.#checkedNodeFormPayloads.clear()
119+
}
120+
111121
this.#checkedNodeIds.set(nodeId, nodeInfo.checkedValue)
112122
const payload: {path: string[]; value?: string} = {path: nodeInfo.path}
113123
const dataValue = node.getAttribute('data-value')
@@ -391,12 +401,14 @@ export class FilterableTreeViewElement extends HTMLElement {
391401
// Applies a previously captured expansion snapshot to the current tree.
392402
#applyExpansionSnapshot(snapshot: Map<string, boolean>) {
393403
for (const [nodeId, wasExpanded] of snapshot) {
394-
const treeitem = this.querySelector<HTMLElement>(
395-
`[role=treeitem][data-node-id="${CSS.escape(nodeId)}"]`,
396-
)
404+
const treeitem = this.querySelector<HTMLElement>(`[role=treeitem][data-node-id="${CSS.escape(nodeId)}"]`)
397405
const subTreeNode = treeitem?.closest('tree-view-sub-tree-node') as TreeViewSubTreeNodeElement | null
398406
if (subTreeNode) {
399-
wasExpanded ? subTreeNode.expand() : subTreeNode.collapse()
407+
if (wasExpanded) {
408+
subTreeNode.expand()
409+
} else {
410+
subTreeNode.collapse()
411+
}
400412
}
401413
}
402414
}

app/components/primer/open_project/filterable_tree_view/sub_tree.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ def with_sub_tree(**system_arguments, &block)
1212
system_arguments[:select_variant] ||= :multiple
1313

1414
unless FilterableTreeView::SUPPORTED_SELECT_VARIANTS.include?(system_arguments[:select_variant])
15-
raise ArgumentError, "FilterableTreeView only supports #{FilterableTreeView::SUPPORTED_SELECT_VARIANTS.map { |v| "`:#{v}`" }.join(", ")} as select_variant"
15+
raise ArgumentError, "FilterableTreeView only supports #{SUPPORTED_SELECT_VARIANTS.map(&:inspect).to_sentence} as select_variant"
1616
end
1717

1818
super(
@@ -27,7 +27,7 @@ def with_leaf(**system_arguments, &block)
2727
system_arguments[:select_variant] ||= :multiple
2828

2929
unless FilterableTreeView::SUPPORTED_SELECT_VARIANTS.include?(system_arguments[:select_variant])
30-
raise ArgumentError, "FilterableTreeView only supports #{FilterableTreeView::SUPPORTED_SELECT_VARIANTS.map { |v| "`:#{v}`" }.join(", ")} as select_variant"
30+
raise ArgumentError, "FilterableTreeView only supports #{SUPPORTED_SELECT_VARIANTS.map(&:inspect).to_sentence} as select_variant"
3131
end
3232

3333
super(
@@ -37,11 +37,11 @@ def with_leaf(**system_arguments, &block)
3737
end
3838

3939
def with_loading_spinner(**system_arguments)
40-
raise ArgumentError, "FilterableTreeView does not support select variants for sub-trees loaded asynchronously. Please make the wole component load asynchronously."
40+
raise ArgumentError, "FilterableTreeView does not support select variants for sub-trees loaded asynchronously. Please make the whole component load asynchronously."
4141
end
4242

4343
def with_loading_skeleton(**system_arguments)
44-
raise ArgumentError, "FilterableTreeView does not support select variants for sub-trees loaded asynchronously. Please make the wole component load asynchronously."
44+
raise ArgumentError, "FilterableTreeView does not support select variants for sub-trees loaded asynchronously. Please make the whole component load asynchronously."
4545
end
4646
end
4747
end

app/controllers/primer/view_components/filterable_tree_view_items_controller.rb

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ module ViewComponents
66
# :nocov:
77
class FilterableTreeViewItemsController < ApplicationController
88
# A node in the demo tree. Leaf nodes have no children.
9-
TreeNode = Struct.new(:id, :label, :children, :all_descendant_ids, keyword_init: true) do
9+
TreeNode = Data.define(:id, :label, :children, :all_descendant_ids) do
1010
def leaf?
1111
children.nil? || children.empty?
1212
end
@@ -23,7 +23,7 @@ def filter(query)
2323
else
2424
filtered_children = children.filter_map { |child| child.filter(query) }
2525
if matches?(query) || !filtered_children.empty?
26-
TreeNode.new(
26+
self.class.new(
2727
id: id,
2828
label: label,
2929
children: filtered_children,
@@ -165,34 +165,26 @@ def self.branch(id:, label:, children:)
165165
def index
166166
query = params[:query].to_s.strip
167167
select_variant = (params[:select_variant].presence || "multiple").to_sym
168-
include_sub_items = params[:include_sub_items] == "true"
169-
checked_ids = Array(params["checked_ids[]"]).map(&:to_s)
170168
nodes = TREE.filter_map { |node| node.filter(query) }
171169

172170
render locals: {
173171
nodes: nodes,
174172
query: query,
175-
select_variant: select_variant,
176-
include_sub_items: include_sub_items,
177-
checked_ids: checked_ids
173+
select_variant: select_variant
178174
}
179175
end
180176

181177
def async_form_tree
182178
query = params[:query].to_s.strip
183179
name = params[:name].to_s.presence || "characters"
184-
include_sub_items = params[:include_sub_items] == "true"
185-
checked_ids = Array(params["checked_ids[]"]).map(&:to_s)
186180
nodes = TREE.filter_map { |node| node.filter(query) }
187181
builder = ActionView::Helpers::FormBuilder.new("", nil, view_context, {})
188182

189183
render locals: {
190184
nodes: nodes,
191185
query: query,
192186
name: name,
193-
builder: builder,
194-
include_sub_items: include_sub_items,
195-
checked_ids: checked_ids
187+
builder: builder
196188
}
197189
end
198190
end

previews/primer/open_project/filterable_tree_view_preview/link_nodes.html.erb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,17 @@
22
tree_view_arguments: { node_variant: :anchor },
33
filter_mode_control_arguments: { hidden: true }
44
)) do |tree| %>
5-
<% tree.with_sub_tree(label: "Cloud Services", select_variant: :none, expanded: expanded, href: "https://en.wikipedia.org/wiki/Cloud_computing", target: "blank") do |cloud| %>
6-
<% cloud.with_leaf(label: "OpenProject", select_variant: :none, href: "https://www.openproject.org", target: "blank") do |node| %>
5+
<% tree.with_sub_tree(label: "Cloud Services", select_variant: :none, expanded: expanded, href: "https://en.wikipedia.org/wiki/Cloud_computing", target: "_blank") do |cloud| %>
6+
<% cloud.with_leaf(label: "OpenProject", select_variant: :none, href: "https://www.openproject.org", target: "_blank") do |node| %>
77
<% node.with_trailing_visual_icon(icon: :"link-external") %>
88
<% end %>
99

10-
<% cloud.with_leaf(label: "Hetzner", select_variant: :none, href: "https://www.hetzner.com", target: "blank") do |node| %>
10+
<% cloud.with_leaf(label: "Hetzner", select_variant: :none, href: "https://www.hetzner.com", target: "_blank") do |node| %>
1111
<% node.with_trailing_visual_icon(icon: :"link-external") %>
1212
<% end %>
1313
<% end %>
1414

15-
<% tree.with_leaf(label: "GitHub", select_variant: :none, href: "https://github.com", target: "blank") do |node| %>
15+
<% tree.with_leaf(label: "GitHub", select_variant: :none, href: "https://github.com", target: "_blank") do |node| %>
1616
<% node.with_trailing_visual_icon(icon: :"link-external") %>
1717
<% end %>
1818
<% end %>

test/components/component_test.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ def test_registered_components
231231
"Primer::Alpha::TreeView::IconPair",
232232
"Primer::Alpha::TreeView::Icon",
233233
"Primer::Alpha::TreeView::LeadingAction",
234+
"Primer::Alpha::TreeView::TrailingAction",
234235
"Primer::Alpha::TreeView::LeafNode",
235236
"Primer::Alpha::TreeView::LoadingFailureMessage",
236237
"Primer::Alpha::TreeView::Node",

0 commit comments

Comments
 (0)