Fix legend placement by selecting the correct bookmark icon - #435
Merged
Conversation
- Updated to use querySelectorAll() to get all bookmark icons - Select the last icon using array indexing [length - 1] - Added #content scoping as a preventive measure to limit search scope - This ensures the legend is positioned correctly when multiple bookmarks exist - Addresses issue identified in PR haru#429 where multiple bookmark icons caused incorrect legend placement
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The selector change addresses a multi-bookmark regression but isn’t covered by a test that includes multiple .icon-bookmarked-project elements, and the test update introduces trailing whitespace that may fail linting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes incorrect placement of the AI Helper legend item on projects#index when multiple .icon-bookmarked-project elements exist, by selecting the correct (last) bookmark icon in the main content area.
Changes:
- Switch legend anchor lookup from
querySelector()toquerySelectorAll('#content .icon-bookmarked-project')and select the last match. - Update the JS DOM test setup to include a
#contentwrapper matching the new selector scope.
File summaries
| File | Description |
|---|---|
| assets/javascripts/project_legend/ai_helper_project_legend.js | Selects the last bookmarked-project icon within #content before appending the AI Helper legend item. |
| test/javascript/project_legend/ai_helper_project_legend.test.js | Adjusts DOM fixture to include #content so the selector works under test. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ter match Redmine’s structure - pass content container explicitly to test helpers - model project hierarchy with an inner container for bookmark icon parent checks - clarify selector limitation in legend script comments
Owner
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses an issue discovered in the implementation of PR #429.
The legend icon placement wasn't correct when one or more projects were bookmarked.
Problem
When one or more projects are bookmarked, the AI Helper legend icon would be appended to the first bookmark icon found, causing incorrect positioning.
Solution
Updated the selector logic to
querySelectorAll():Primary fix: Uses
querySelectorAll('#content .icon-bookmarked-project')to get all bookmark icons and selects the last one with[length - 1], ensuring correct placement even with multiple bookmarked projects.Preventive measure:
#contentscoping limits the search to the main content area, reducing the risk of selecting unintended elements.