[DREAM-801] Extend BorderBoxList header API - #24643
Conversation
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
dd3996c to
eb88b90
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
lookbook/previews/open_project/common/border_box_list_component_preview.rb:157
- The Lookbook preview uses
html_safeinline, which can normalize unsafe patterns for consumers copying examples. Prefer building the link with Rails helpers (e.g.,link_to/tag.a) so the example is safe-by-default while still demonstratingwith_title.
header.with_title do
'<a href="#" class="Link--primary no-underline">Linked delivery plan</a>'.html_safe
end
app/components/open_project/common/border_box_list_component/empty_state.rb:104
- Using a truthy check for
@action_labelwill render a primary action even when the label is an empty string, creating an unlabeled control. Consider checking presence (e.g.,present?) before rendering the CTA; also consider duplicating@action_argumentsdefensively to avoid unexpected mutation if downstream code modifies the hash.
if @action_label
action = blankslate.with_primary_action(**@action_arguments)
action.with_leading_visual_icon(icon: @action_icon) if @action_icon
action.with_content(@action_label)
end
lookbook/previews/open_project/common/border_box_list_component_preview/with_header_drag_handle.html.erb:7
- This template starts at column 1 but the overall formatting/indentation is inconsistent (e.g., argument alignment and template body indentation). Consider normalizing indentation to match surrounding Lookbook templates to keep previews easy to read/maintain.
<%= render OpenProject::Common::BorderBoxListComponent.new(
container: "border-box-list-header-drag-handle-preview",
padding:,
header_padding:,
collapsible:
) do |list| %>
<% list.with_header(title: "Reorderable section", count: true, show_drag_handle: true) %>
eb88b90 to
f52cb1a
Compare
f52cb1a to
053e626
Compare
053e626 to
0d5d9e9
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
98e1458 to
c8b4051
Compare
b795009 to
60af92a
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
Adds the title and action-icon-button header slots and reworks header and row menus so callers can restyle the default trigger or provide their own show button. https://community.openproject.org/wp/DREAM-697
Renders a generic blankslate whenever an itemless list does not configure the empty-state slot, so every consumer keeps a stable drop target and screen-reader announcement without bespoke copy. https://community.openproject.org/wp/DREAM-697
Documents the `show_drag_handle` header option of `BorderBoxListComponent` in Lookbook, pairing the header handle with per-row `DragHandle` columns as reorderable lists render it. https://community.openproject.org/wp/DREAM-697
Reverts commit 64ae48d (AGILE-309): rendering the empty state alongside populated rows and hiding it with CSS sibling selectors made every consumer carry structural :has()/:only-child compensations. A follow-up commit reintroduces the instant blankslate as an opt-in, controller-owned behavior. The independent Turbo fix 6aa9ff1 from the same PR is preserved. https://community.openproject.org/wp/DREAM-801
Grouped per-type and per-section lists showed a generic blankslate under every empty group once the default empty state landed. A single policy replaces boolean opt-outs: :static keeps the generic default, :none suppresses it for grouped outliers, :dynamic reserves the client-side lifecycle for sortable and filtered lists. https://community.openproject.org/wp/DREAM-801
Lists whose rows change client-side (drag and drop, filtering) need the blankslate to appear without a server round-trip. A list-owned Stimulus controller now inserts a placeholder row cloned from an inert template whenever no visible row remains, replacing the reverted CSS sibling selectors. Producers stay untouched: the controller observes row mutations instead of being called. https://community.openproject.org/wp/DREAM-801
A :dynamic list populated at render time with no declared empty state left its parked template prototype contentless, because configure_empty_state! only injected the generic default when the list was truly empty. Draining such a list to zero rows client-side then cloned a blank placeholder instead of a real blankslate. Dynamic lists now get the generic default whenever no empty-state slot is declared, regardless of item count; :static keeps its existing default-only-when-empty behavior.
Restores the instant blankslate from AGILE-309 for the sprint, bucket and inbox lists through the lifecycle controller instead of the reverted CSS rules. https://community.openproject.org/wp/DREAM-801
Headers whose action buttons are hidden while filtering shrank by the button height. The heading line now reserves the medium control height, replacing the caller-side title padding the migrations dropped. https://community.openproject.org/wp/DREAM-801
Replaces the two ERB captures with Primer::ConditionalWrapper and direct slot emission. Slot content memoizes on first render, so the template prototype and the live empty row can both emit the slot without a shared capture. The wrapper gains Primer's inert data-view-component attribute; the controller and target contract is unchanged. https://community.openproject.org/wp/DREAM-801
Replaces the hand-rolled MutationObserver lifecycle with stimulus-use's useMutation, which owns observer creation and disconnect teardown. Events stay off via dispatchEvent: false so the placeholder insertion does not emit bubbling mutate events the manual observer never had. https://community.openproject.org/wp/DREAM-801
553e48d to
14184ed
Compare
|
@bsatarnejad thanks for the feedback. By default empty states are always rendered, but this can be controlled via an
These API changes are a bit more rushed than I would like, so we probably need to re-evaluate soon. We've discussed the |
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
Ticket
https://community.openproject.org/wp/DREAM-801
What are you trying to accomplish?
First slice of the DREAM-697 split (supersedes the monolithic draft PR #23812): extends the shared
OpenProject::Common::BorderBoxListComponentAPI that the consumer migrations later in this stack need.header.with_titlefor linked or composed title content inside the configured heading element.header.with_action_icon_buttonfor icon-only header actions.BorderBoxListComponent::Menu: the default kebab trigger stays automatic; callers can restyle it withbutton_arguments:or provide their ownmenu.with_show_button. The Backlogs bucket/sprint callers adopt the new argument.with_empty_stategains an optional call-to-action (action_label:,action_icon:,action_arguments:).empty_state_behavior:option::static(default) renders the declared or generic blankslate only while the list is empty;:nonesuppresses it (used by the grouped WP types and project-attributes sections, where a blankslate under every empty group is noise);:dynamicadditionally parks a<template>prototype next to the list and a smallborder-box-listStimulus controller shows it client-side whenever no visible row remains (Backlogs opts in). This reverts the always-rendered empty-state row and its CSS sibling selectors from Implementation/agile 309 improve the dragging experience with the frame reloading #23950 — thedata-turbo-permanentfix from that PR is kept.min-height: var(--control-medium-size), so headers no longer shrink when filtering hides their action buttons (review finding on [DREAM-803] Migrate wiki, OAuth and project settings lists to BorderBoxList #24645).The originally drafted
label:header-action deprecation was dropped — superseded by DREAM-780 (PR #24560).What approach did you choose and why?
PR #23812 stalled as a 55-file review; this stack splits it into the shared API change (this PR) followed by one consumer-migration batch per PR. The empty-state policy replaces boolean opt-outs so invalid combinations (hidden + persistent) cannot be expressed, and client-side visibility lives in one list-owned controller (
useMutation) instead of:only-child/:has()CSS. As discussed in review, the option may be revisited once #24669 (DREAM-805) lands — e.g. inferring:dynamicandinteractive:from the declarative sortable wiring.Visual comparisons
Show baseline/candidate screenshots
Baseline is shown on the left; the candidate stack is shown on the right. (Screenshots predate the empty-state rework; regeneration pending.)
Lookbook: custom header content
Lookbook: header drag handle
Lookbook: default empty state
Merge checklist