[DREAM-798] TreeView is misaligned when using single select and async loading - #522
Conversation
🦋 Changeset detectedLatest commit: 6db6bf5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR addresses a TreeView UI alignment bug in the alpha async-loading + single-select variant, and renames the Lookbook preview parameter that controls the nested expanded state to better reflect intent.
Changes:
- Renamed the async TreeView preview/controller param from
action_menu_expandedtonested_child_expandedand updated all call sites/tests. - Adjusted single-select checkmark markup/CSS so it aligns correctly (and added a component test for the rendered checkmark).
- Added a changeset documenting the patch-level fix.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/system/alpha/tree_view_test.rb | Updates system test to use the renamed preview param. |
| test/system/alpha/action_menu_test.rb | Avoids potential stale-element access by caching aria-disabled before key activation. |
| test/components/primer/alpha/tree_view_test.rb | Adds a unit test asserting the single-select checkmark renders as the trailing visual. |
| static/classes.json | Registers the TreeViewItem-singleSelectCheckmark class for the TreeView component. |
| previews/primer/alpha/tree_view_preview/async_alpha.html.erb | Passes the renamed param through to the async items endpoint. |
| previews/primer/alpha/tree_view_preview.rb | Renames the preview parameter and updates the preview locals. |
| app/views/primer/view_components/tree_view_items/async_alpha.html_fragment.erb | Uses the renamed local (nested_child_expanded) to expand the nested sub-tree. |
| app/controllers/primer/view_components/tree_view_items_controller.rb | Renames the async-alpha param parsed from request params. |
| app/components/primer/alpha/tree_view/node.html.erb | Moves the single-select checkmark class onto the wrapper visual element. |
| app/components/primer/alpha/tree_view.pcss | Adds alignment styling for the single-select checkmark visual. |
| .changeset/calm-trees-align.md | Documents the patch release note for the alignment fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
myabc
left a comment
There was a problem hiding this comment.
Although this solution works, it's needlessly complicated and ends up using a CSS class (TreeViewItem-singleSelectCheckmark) in a way that is inconsistent with the rest of the component.
I believe this can be simplified considerably by changing tree_view.pcss:287 as follows:
- align-self: baseline;
+ align-self: flex-start;and then reverting the other changes in tree_view.pcss and node.html.erb (see suggestion).
It might be cleaner/easier to make a new PR with those simplified changes and then extract/cherry-pick the nested_child_expanded rename separately, if that's still deemed necessary.
| <span class="TreeViewItemVisual TreeViewItem-singleSelectCheckmark" aria-hidden="true"> | ||
| <%= render(Primer::Beta::Octicon.new(icon: :check)) %> |
There was a problem hiding this comment.
| <span class="TreeViewItemVisual TreeViewItem-singleSelectCheckmark" aria-hidden="true"> | |
| <%= render(Primer::Beta::Octicon.new(icon: :check)) %> | |
| <span class="TreeViewItemVisual" aria-hidden="true"> | |
| <%= render(Primer::Beta::Octicon.new(icon: :check, classes: "TreeViewItem-singleSelectCheckmark")) %> |
|
|
||
| &.TreeViewItem-singleSelectCheckmark { | ||
| align-self: flex-start; | ||
| } |
There was a problem hiding this comment.
| &.TreeViewItem-singleSelectCheckmark { | |
| align-self: flex-start; | |
| } |
|
HDinger
left a comment
There was a problem hiding this comment.
Works nicely 👍
I am not a big fan of the wording for nested_child_expanded though as is grammatically wrong. I'd prefer either nested_children_expanded or expand_nested_children.
| keyboard.type(*keys) | ||
|
|
||
| return if !expect_focus_change || current_item["aria-disabled"] | ||
| return if !expect_focus_change || current_item_disabled |
There was a problem hiding this comment.
This does not add functional changes as far as I can tell? I'd like to keep the changes to Primer components as minimal as possible to avoid merge conflicts with future changes.
What are you trying to accomplish?
Rename the 'Action Menu Expanded' param
Fix misalignment between text and chevron
Screenshots
Integration
List the issues that this change affects.
Closes https://community.openproject.org/wp/DREAM-798
Risk Assessment