[74625] Make project hierarchy collapsable in the global project selector#23137
Conversation
Deploying openproject with ⚡ PullPreview
|
f542b25 to
f78ae09
Compare
|
@lindenthal The PullPreview will not work here (yet) since the changes are based on the local changes to the FilterableTreeView component. Since that is not released yet, this PR doesn't know anything about the async loading and will just fail. |
There was a problem hiding this comment.
Pull request overview
Replaces the Angular-based global project selector (opce-header-project-select) with a server-rendered ViewComponent (Header::ProjectSelectComponent) backed by the new async Primer::OpenProject::FilterableTreeView. A new Header::ProjectsController#index endpoint returns the tree fragment (visible, active projects ordered by lft, with the current project's hierarchy preserved and a 300-row cap), and the obsolete Angular component, list, sass, HTML, helper, and locale keys are deleted. A small Stimulus controller persists the "all/favorites" filter mode in localStorage, and the FilterableTreeView lookbook docs are extended to document async mode and the new select_variant: :none (anchor) usage. Main-menu sass is tweaked so the embedded TreeView is not over-styled by the menu rules.
Changes:
- Introduce backend
Header::ProjectSelectComponent+Header::ProjectsControllerand node partials returning aPrimer::Alpha::TreeViewfragment for an asyncFilterableTreeView. - Remove the Angular
OpHeaderProjectSelect*component, related sass/HTML, insert-in-list helper, and obsoletejs.label_project_list/include_workspaces.search_placeholderlocale keys; add newheader_project_select.*keys. - Add a Stimulus controller to persist filter-mode selection, scope main-menu sass to avoid styling TreeView, and document async FilterableTreeView mode +
select_variant: :none.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/redmine/menu_manager/top_menu/projects_menu.rb | Render new ViewComponent instead of Angular custom element. |
| app/components/header/project_select_component.{rb,html.erb,sass} | New Primer Overlay-based project select trigger and body. |
| app/controllers/header/projects_controller.rb | New endpoint returning a tree fragment of visible projects (with filter/search). |
| app/views/header/projects/index.html.erb, _node.html.erb | Tree fragment + recursive node partial with anchor nodes, star/leave icons. |
| config/routes.rb | Add namespace :header { resources :projects, only: :index }. |
| config/locales/en.yml, config/locales/js-en.yml | Add header_project_select.* keys; remove obsolete js.label_project_* and include_workspaces.search_placeholder. |
| frontend/src/stimulus/controllers/header-project-select.controller.ts, setup.ts | Persist last filter mode in localStorage and re-apply it on open. |
| frontend/src/global_styles/layout/_main_menu.sass | Scope generic ul/a rules so they don't apply to TreeView. |
| frontend/src/global_styles/primer/_overrides.sass | Layout tweak for the project select FilterableTreeView body. |
| frontend/src/app/shared/components/header-project-select/** | Delete Angular component, template, sass, list, insert-in-list helper. |
| frontend/src/app/shared/components/grids/widgets/project-favorites/* | Move "no favorites" styles to the widget that still uses them. |
| frontend/src/app/app.module.ts | Drop registration of the deleted Angular component and custom element. |
| frontend/package-lock.json | Updated @primer/behaviors, @github/include-fragment-element, @github/catalyst and license metadata. |
| lookbook/docs/components/tree-view/filterable-tree-view.md.erb | Document async (src:) mode and select_variant: :single/:none. |
| spec/features/menu_items/top_menu_item_spec.rb | Update I18n keys used by the test (see review comment). |
| app/components/_index.sass | Import the new component's sass. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| private onFilterModeClick = (event:MouseEvent):void => { | ||
| const button = (event.target as HTMLElement).closest<HTMLElement>('[data-name]'); | ||
| if (button?.dataset.name) { | ||
| localStorage.setItem(STORAGE_KEY, button.dataset.name); | ||
| } | ||
| }; |
There was a problem hiding this comment.
It took me a while to understand this comment, but I believe the issue is that any nested element inside the project selector that renders a data-name attribute (a pretty common attribute name), could interfere with the implementation.
f354e8a to
dd3bf70
Compare
|
hi @HDinger,
|
9bcc8c4 to
cb24cc9
Compare
There was a problem hiding this comment.
This works well - and "feels" snappy. 💯
That said, given what a critical part of the UI this is, I've striven to be extra stringent in this review.
Inline comments that I consider especially important have been marked with a
- visibility leak in
merge_with_ancestors - favourites mode breaking hierarchy
- lack of guards on
localStorage - removal of the project menu keyboard shortcut (i.e.
accesskey="5")
Also very important, but not mentioned directly inline:
- eager loading of projects:
tree-viewand the overlay are now loaded on every page. Previously, an API call to/api/v3/projectswas only made on opening the project selector.
Those issues don't necessarily need to block this PR, but we should be prepared to handle any performance regressions.
Some things I'm not fond of:
- lack of specs: this is my biggest concern. There are no controller/request specs in this PR. While the feature specs are good enough to verify the basic functionality, we need unit-style specs to iron out the edge cases and make it easier for future devs to verify fixes for the inevitable regressions.
- complexity in the views, e.g.
_node.html.erb. Also our reliance on helpers. I don't have a good solution for this though. - amount of logic in the controllers
Some suggestions: (for either this PR or a follow-up)
- extract tree-building into a service —
Header::ProjectTreeBuilderwith#call(projects, current_project_id:, matching_ids:)returning the nested structure. Makes it unit-testable without controller overhead and keeps the controller thin. (I can imagine @ulferts also wanting this 😄 ) - server-side filter mode via cookie/session — Instead of
localStorage(with all its pitfalls), persist filter mode server-side. The Stimulus controller becomes zero lines. - request specs for
Header::ProjectsController. Given this endpoint serves every page load, it deserves:- Visibility filtering (user sees only permitted projects)
- Ancestor inclusion (search + favourites)
- Anonymous user behaviour
current_project_idedge cases (0, nil, invalid, archived)- The 300-project cap behavior
Claude also came up with this suggestion, which I'd file under "future optimisations"
- Consider collection rendering for
_node.html.erb— If tree depth is bounded (which it effectively is at 300 nodes), a flat iteration with depth tracking could replace recursive partial rendering, reducing ERB overhead.
| anchor_side: :outside_bottom, | ||
| anchor_align: :start, | ||
| size: :medium_portrait, | ||
| padding: :none |
|
@HDinger regarding this comment:
Do you have a local BIM environment to reproduce this? |
…ject-hierarchy-collapsable-in-the-global-project-selector
5c181e0 to
93f6639
Compare
…guarded local storage
93f6639 to
34d9306
Compare
…cts when it is actually opened
34d9306 to
628bb85
Compare
…new project selector changes made it so bad that the test broke because the plus icon was overlapping the checkbox
…ject-hierarchy-collapsable-in-the-global-project-selector
…ject-hierarchy-collapsable-in-the-global-project-selector
|
Hi @lindenthal
Thanks for the feedback. This is very interesting as I could not reproduce locally, but on the PullPreview instance. I will double check what happened.
Yes, we are aware of that and decided to tackle in a follow-up PR 👍
There are ways to quite easily re-render the whole content of the dropdown with expanded nodes. I guess the challenge is rather in the details like keeping the search term and the current scroll position. |







Warning
this relies on the changes from opf/primer_view_components#451
Ticket
https://community.openproject.org/wp/74625
What are you trying to accomplish?
Features
Housekeeping
Screenshots