Conversation
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
3 times, most recently
from
0a230ad to
5282199
Compare
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
5282199 to
b8fa4bb
Compare
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
b8fa4bb to
7a142b0
Compare
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
2 times, most recently
from
e1c2623 to
9e9c053
Compare
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
9e9c053 to
2777895
Compare
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
bebf10e to
25e2c19
Compare
myabc
added
feature
javascript
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
25e2c19 to
7c350e7
Compare
There was a problem hiding this comment.
Hey @myabc I really like where this is heading. Doesn't look like Y2K any more.
I didn't do a complete and thorough review but rather read through it all and remarked on things here and there.
There are a number of general topics I'd like to raise to discuss later:
- Currently, the implementation is based on turbo streams. Backlogs are loaded individually when they are changed which, even with the limited set of scenarios provided does not always work and leads to complicated code. I have never done it and don't know if it will work but I would be interested in your opinion on using turbo drive or frame to load the whole page. To avoid undesired flickering and state changes, morphing would need to be used and the state would need to be sent to the backend (or morphing disabled on some parts of the application via JS). This would greatly simplify the backend/frontend communication I believe. Reloading the page upon changes in the split view e.g. would not need a lot of knowledge on what was changed. With the current approach, on a change to version, two backlogs would need to be reloaded. For the move action, this already seems to be failing and that is only within the stimulus controlled parts of the application. E.g. when moving the last story out of a backlog, it receives a way to big "Sprint backlog is empty" skeleton. All the other empty sprints are updated as well. This can be fixed but I see that as a warning sign of things to come:
- As mentioned above, changes in the work package split view are not reflected in the backlog view. Additionally, one can change the work package in a way that it will disappear from the page. Currently this can be done by changing the type to a non story type or by changing the version to one that is not on the page. The first problem will likely disappear once the setting is removed.
- With the split view opening, there is very little space
Then there are some bugs I noticed when trying out the changes:
- Moving the a story to the backlog and trying to move it again right away does not work
- When opening the split view, the backlogs area sometimes did not allow to scroll horizontally. That happens after drag and drop, maybe?
- Opening the split view breaks the right hand side margin so backlog and button are glued to the right hand side:
- We might consider opening a folded backlog when dragging. At least after hovering over it for some time
- The menu item says "Edit title" when in fact I can edit more attributes:
| } else { | ||
| this.openSplitPane(); | ||
| } | ||
| } |
There was a problem hiding this comment.
If the approach is kept, this part of the code needs to be dry-ed
There was a problem hiding this comment.
@ulferts I'm not 100% sure how to DRY this code. We might be able to make this into a more generic ListItemController if/when we extract BorderBoxList (see OP # 71402). / @HDinger
There was a problem hiding this comment.
@ulferts I suppose we could look at moving the openSplitPane/openFullPane methods to the BacklogsController. Since StoryController defines an idValue, there is probably no need for the data-backlogs--story-split-url and data-backlogs--story-full-url attributes.
| blankslate.with_heading(tag: :h4).with_content(t(".product_backlog.blank_slate_title")) | ||
| blankslate.with_description_content(t(".product_backlog.blank_slate_description")) | ||
| end | ||
| %> |
There was a problem hiding this comment.
Moving the logic to the component (backlog.sprint_backlog?) would simplify the template since the only difference seems to be in the I18n keys.
modules/backlogs/app/components/backlogs/backlog_header_component.rb
Outdated
Show resolved
Hide resolved
| mem | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Some of these methods were used for performance optimizations. Without them, some n+1 queries are introduced:
Eager loading status and type when the stories are loaded should fix that.
There was a problem hiding this comment.
@ulferts can you check if this is still an issue?
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
7c350e7 to
e8486ab
Compare
myabc
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
e8486ab to
e61b486
Compare
| end | ||
| %> | ||
| <% else %> | ||
| <div class="op-backlogs-container" data-controller="generic-drag-and-drop"> |
There was a problem hiding this comment.
FYI, the class and id naming is a bit inconsistent here. @HDinger and I picked these names while pairing. Better suggestions are welcome.
dombesz
force-pushed
the
feature/57688-primerize-backlogs-classic
branch
from
3fe8c0d to
3b3bdbd
Compare
HDinger
left a comment
HDinger
left a comment
There was a problem hiding this comment.
As disucssed, I just looked at the frontend part briefly. I guess there are some optimization with regards to responsivness that can be done, but those are no blockers. Further, also as discussed, I am no fan of the duplication of the collapsible header but it is fine to extract those changes in a separate step.
Additionally, I found some other things:
- The breakpoint to switch the column layout in the editbale header should be earlier:
- When trying to set an end date to a verion, I ran into this error:
| @@ -0,0 +1,18 @@ | |||
| <div class="position-relative mx-auto" style="height:60vh"> | |||
There was a problem hiding this comment.
I am not a big fan of using these classes directly. Primer already changed them in the past and the advantage of system_arguments is that we don't have to care about this. Further, inline styles are also kind of an anti-pattern for me.
So my suggestion would be, to add a class and put all those styles there.
There was a problem hiding this comment.
Primer's CSS is quite stable at this point, so I think it's ok to use utility classes directly. This was copy-pasted from the Budgets widget PR, so perhaps I can DRY that up once that is merged.
| .Box-row--focus-gray | ||
| &:focus-visible | ||
| background-color: var(--bgColor-muted) | ||
|
|
||
| .Box-row--focus-blue | ||
| &:focus-visible | ||
| background-color: var(--bgColor-accent-muted) |
There was a problem hiding this comment.
I don't quite see why this is needed. The focus border already indicates the selection. If we need it, then it should be part of the PVC repo, shouldn't it?
There was a problem hiding this comment.
@HDinger This override could be upstreamed. Upstream currently uses .navigation-focus (which presumably is added with JavaScript?) rather than the newer :focus-visible pseudo-selector.
.Box-row--focus-gray {
&.navigation-focus {
background-color: var(--bgColor-muted);
}
}
.Box-row--focus-blue {
&.navigation-focus {
background-color: var(--bgColor-accent-muted);
}
}
EinLama
left a comment
EinLama
left a comment
There was a problem hiding this comment.
Looks and feels so much better than the old backlog module 😁
I did not have a close look at styling or frontend details. For the controllers and general look and feel, I shall happily approve ✅
Please mind that I do not intend to overrule any open change requests by other people. This is just an approval for the code parts that are important to me.
When only one of start/end date is set, show an en-dash placeholder for the missing date instead of displaying the single date alone (which looked like a milestone). Remove .compact from date_range so nils flow through to format_date_range, which now handles them explicitly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract `update_header_component_via_turbo_stream` helper to remove repeated backlog-build + turbo-stream-update block from `edit_name`, `show_name`, and `update` actions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Make it private (matching the parent class) and remove unnecessary guards since params are always present in the routes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract `load_story` before_action and `replace_backlog_component_via_turbo_stream` helper to remove repeated story lookup and backlog component rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
We should generate friendly URLs using the project identifier, not project id. While this inconsistency is pervasive in the OpenProject codebase, this commit aims to at least make links within the backlogs module a consistent format.
Important
This PR supersedes #21711 , which was an experiment based on Primer React
Ticket
https://community.openproject.org/wp/57688
What are you trying to accomplish?
This PR delivers a comprehensive visual and technical revamp of the Backlogs index page (Master Backlogs view) as part of our ongoing migration to the Primer design system.
Summary of Changes
🎯 Backlogs index page — Complete Rewrite
The main focus of this PR is modernizing the Backlogs index page (
/projects/:project_id/backlogs):New ViewComponent Architecture
Rewrote the entire frontend using 7 new ViewComponents:
BacklogComponent— Main container for each sprint/backlogBacklogHeaderComponent— Collapsible header with sprint info and actionsBacklogMenuComponent— Sprint-level action menuStoryComponent— Individual story row with drag handleStoryMenuComponent— Story-level action menuCollapsibleComponent— Reusable collapsible wrapper (ℹ️ this is temporary - changes will be upstreamed intoPrimer::OpenProject::BorderBox::CollapsibleHeader)SprintPageHeaderComponent— Page header for sprint viewsBuilt on Primer's
BorderBoxcomponent for consistent stylingAdded comprehensive component specs for all new components
Modern CSS & Responsive Design
.Box-rowelements (Primer overrides) (ℹ️ again this is temporary and should be upstreamed into PVC)Improved Interactions
📊 Burndown chart — Rewritten with Chart.js
BurndownChartComponentregistered as custom element🧹 Cleanup & Technical Improvements
SetAttributesServiceScreenshots
Backlogs index page
Burndown chart
Taskboard
Not configured page
What approach did you choose and why?
Turbo Frames and Streams: we use both in this iteration. Streams make sense when we control the update and know which areas to "surgically" update (e.g. Sprint name and dates). Frames make sense when the updates happen via a completely different part of the codebase (e.g. details pane)
Chart.js over jquery.flot: jquery.flot is unmaintained and doesn't support modern responsive design. Chart.js is actively maintained, has better accessibility, and integrates well with our existing frontend stack.
CSS Grid + Container Queries: These modern CSS features allow components to adapt based on their container size rather than viewport size, which is essential for the Backlogs page where columns can vary in width.
What this PR doesn't do
Provide robust / "full" accessibility: this PR includes basic keyboard support, but does not implement all WAI recommendations.
What this PR won't do
modules/backlogs.Merge checklist