[DREAM-804] Migrate meetings, costs and work package tab lists to BorderBoxList - #24646
Conversation
a054b81 to
151356d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (6)
modules/meeting/app/components/meetings/participants/list_component.rb:49
- This predicate re-queries/iterates over
@meeting.participantseven though@meeting_participantswas already built ininitialize. Using@meeting_participantshere avoids duplicate association loads and keeps behavior consistent with the sorted participant list (e.g.,!@meeting_participants.all?(&:attended?) && @meeting.in_progress?).
def show_mark_all_attended?
!@meeting.participants.all?(&:attended?) && @meeting.in_progress?
end
modules/meeting/app/components/meetings/participants/list_component.html.erb:19
- The header count uses
@meeting.participants.count, which can trigger an extra COUNT query even though@meeting_participantshas already been computed. Prefer using@meeting_participants.size(or@meeting_participants.length) so the count is derived from the already-loaded/sorted list and avoids extra DB work.
list.with_header(
title: t("meeting.participants.label.participants"),
count: @meeting.participants.count,
color: :muted
) do |header|
modules/meeting/app/components/meetings/participants/list_component.html.erb:43
list.with_empty_stateis declared unconditionally. IfBorderBoxListComponentdoes not automatically suppress the empty state when items exist, this will render both the empty-state and the participant rows simultaneously. If the component API expects callers to only declare empty-state when empty, wrap this in anif @meeting_participants.none?/if @meeting_participants.empty?guard (or switch to the component’s documented pattern for conditional empty-states).
list.with_empty_state(
title: I18n.t("meeting.participants.blankslate.heading"),
description: I18n.t("meeting.participants.blankslate.description"),
icon: :people
)
@meeting_participants.each do |participant|
list.with_item do
render(Meetings::Participants::BoxRowComponent.new(meeting: @meeting, participant:))
end
end
modules/costs/app/components/projects/settings/cost_types/index_component.html.erb:38
- The empty-state is declared unconditionally after rendering items. If
BorderBoxListComponentdoesn’t internally hide the empty-state when items are present, this will display an empty-state alongside the populated list. If the API requires explicit conditional rendering, guard the empty-state withif cost_types.none?(orif empty?) so it only appears when there are no items.
<% cost_types.each do |cost_type| %>
<% list.with_item do %>
modules/costs/app/components/projects/settings/cost_types/index_component.html.erb:97
- The empty-state is declared unconditionally after rendering items. If
BorderBoxListComponentdoesn’t internally hide the empty-state when items are present, this will display an empty-state alongside the populated list. If the API requires explicit conditional rendering, guard the empty-state withif cost_types.none?(orif empty?) so it only appears when there are no items.
<% list.with_empty_state(
title: I18n.t("cost_types.settings.cost_types.heading"),
description: I18n.t("cost_types.admin.cost_type_projects.no_projects.description"),
icon: :checklist
) %>
spec/components/work_package_relations_tab/index_component_spec.rb:61
- The spec used to assert the presence of the group test selector (
op-relation-group-parent) but that assertion was removed. The component still setstest_selector: \"op-relation-group-#{relation_group.type}\", so keeping an explicit expectation for the test selector helps prevent regressions in DOM hooks relied upon by system tests/JS. Consider reintroducing an assertion for theop-relation-group-parentselector in this example.
it "renders the relations group with the parent work package in it" do
expect(render_component).to have_list "Parent"
151356d to
f2f00a3
Compare
f2f00a3 to
14df6ca
Compare
14df6ca to
37559d2
Compare
37559d2 to
2c4d40e
Compare
77a4f41 to
4a329b4
Compare
d245402 to
01612e5
Compare
| <% end %> | ||
| <% end %> | ||
|
|
||
| <% list.with_empty_state( |
There was a problem hiding this comment.
“Cost types” now appears both in the box header and again as the blankslate heading. Previously it appeared only in the blankslate. Is it intended?
h2 changed to h4, is it correct?
There was a problem hiding this comment.
h2 changed to h4, is it correct?
Yes. This should be level 4, I believe (the page heading is level 2)
There was a problem hiding this comment.
“Cost types” now appears both in the box header and again as the blankslate heading. Previously it appeared only in the blankslate. Is it intended?
I think that's ok for now. We will probably need to do some small design follow ups but need feedback from the UX team first.
c1e031d to
0d0a2e9
Compare
0d0a2e9 to
893effa
Compare
893effa to
3207290
Compare
3207290 to
79ab90e
Compare
62171fe to
69ec480
Compare
69ec480 to
c00174e
Compare
Uses the title slot for meeting links and shared rows for agendas. https://community.openproject.org/wp/DREAM-697
Lets a list header show a hierarchy trail in the title position, as the departments detail box needs. The title stays mandatory and turns into a visually hidden heading, so the list keeps its accessible name and heading navigation. Collapsible headers reject the slot because the gem's CollapsibleHeader only accepts h1-h6 title tags.
Replaces the bespoke department box with a BorderBoxList. The hierarchy breadcrumbs render in the list header through the new breadcrumbs slot, the LDAP status becomes a header label, and both bespoke blankslate components retire in favour of the list's empty-state mechanism. https://community.openproject.org/wp/DREAM-697
Moves relation groups and rows onto BorderBoxListComponent. https://community.openproject.org/wp/DREAM-697
Renders the meeting participants box through the shared `BorderBoxListComponent` instead of bespoke `border_box_container` markup. The participants header keeps its title, count, and mark-all action via the title slot, rows render the existing participant row component, and the empty list uses the shared empty-state slot (people icon) in place of the hand-rolled Blankslate branch. Adds a component spec using the shared list examples. https://community.openproject.org/wp/DREAM-697
Renders the project cost-types settings list through the shared `BorderBoxListComponent` instead of a bespoke `Primer::Beta::BorderBox`. The header title, per-row name and toggle switch, and toggle data attributes are preserved; the separate itemless branch is folded into the shared empty-state slot so the list owns its empty state. Adds a component spec using the shared list examples. https://community.openproject.org/wp/DREAM-697
Renders the resource allocations list through the shared `BorderBoxListComponent` instead of a bespoke `Primer::Beta::BorderBox`, keeping the list headerless and preserving the existing per-row list item component and its arguments. The surrounding progress row and the `allocations.any?` guard are kept, so an empty allocation set still renders nothing. Adds a component spec. https://community.openproject.org/wp/DREAM-697
c00174e to
d36799b
Compare
bsatarnejad
left a comment
There was a problem hiding this comment.
Thanks for the clarifications. LGTM 👍🏼
|
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-804
What are you trying to accomplish?
Final slice of the DREAM-697 split: the remaining consumer migrations to
OpenProject::Common::BorderBoxListComponent, one commit per surface.WorkPackageMeetingsTab::MeetingComponentAdmin::Departments::DetailComponent— the hierarchy breadcrumbs render inside the list header through the newwith_breadcrumbsheader slot (the title remains as a visually hidden heading), the LDAP status becomes a header label, and the blank slates adopt the list empty-state mechanism (with the add-department call to action on the new primary-action API), retiring both bespoke blankslate componentsWorkPackageRelationsTab::IndexComponentMeetings::Participants::ListComponent— the heading decomposes intowith_header(title:, count:)plus a mark-all-attended header action, retiringBoxHeaderComponentProjects::Settings::CostTypes::IndexComponentResourceAllocations::ListComponentAfter this lands, DREAM-697's remaining scope is bookkeeping: the multi-column tables reclassified for a future BorderBoxTable and the deferred rich-header surfaces stay out, as documented on the work package.
What approach did you choose and why?
Each migration is a focused vertical slice with its component spec. Stacked on #24645 only for linear review order; the surfaces are independent.
The departments surface needs a hierarchy trail in the box header, so the branch first extends the component with a
with_breadcrumbsheader slot: a thin wrapper aroundPrimer::Beta::Breadcrumbsrendered in the title position, while the mandatory title turns into a visually hidden heading so the list keeps its accessible name and heading navigation. Collapsible headers reject the slot. Documented in Lookbook with a preview.Visual comparisons
Show baseline/candidate screenshots
Baseline is shown on the left; the candidate stack is shown on the right.
Departments
Meeting participants
Project cost types
Work package meetings tab
Work package relations tab
Work package resource allocations
Merge checklist