Conversation
ea638c1 to
7a6f98e
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors static hover cards to use semantic HTML <template> elements instead of hidden DOM containers. The change improves code semantics (templates are designed for content that should be instantiated elsewhere), eliminates the need for custom CSS hiding logic, and streamlines the HoverCardTriggerController to handle both asynchronous (Turbo Frame) and synchronous (template-based) hover cards uniformly.
Key Changes:
- Replaced hidden DOM element pattern with
<template>elements for static hover card content - Removed the
op-hover-card--hidden-containerCSS class (no longer needed since templates are inherently hidden) - Simplified controller logic by using
template.content.cloneNode()instead of element cloning with ID removal
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
lookbook/docs/components/hover-cards.md.erb |
Updated documentation to reflect the new template-based pattern, including corrected examples and clarified behavior around ID usage |
frontend/src/stimulus/controllers/hover-card-trigger.controller.ts |
Refactored to expect HTMLTemplateElement instead of HTMLElement, using template content cloning for cleaner instantiation |
frontend/src/global_styles/content/_hover_cards.sass |
Removed obsolete op-hover-card--hidden-container CSS class definition |
app/components/portfolios/details_component.html.erb |
Migrated from hidden div container to <template> element, moving ID from inner content to template itself |
app/components/portfolios/details_component.sass |
Added width constraint for portfolio hover cards using :has() selector |
toy
left a comment
There was a problem hiding this comment.
Small points primarily about using elements except template.
Not related to this PR, but I'm also wondering about animation, if it is beneficial overall (maybe more when loading remote content), if it can be faster (subjectively .25s feels like a slow appearance, while .1s as just not popping it right away) and, why no animation when hiding the popover (animating popovers can be interesting)?
frontend/src/stimulus/controllers/hover-card-trigger.controller.ts
Outdated
Show resolved
Hide resolved
frontend/src/stimulus/controllers/hover-card-trigger.controller.ts
Outdated
Show resolved
Hide resolved
0d2ce12 to
7f33281
Compare
EinLama
left a comment
There was a problem hiding this comment.
They there @toy
Thank you for the review. It took a long while, but I managed to finally get back to this PR and address all of your feedback 🎉
In the meantime, two new template hover cards were added for project attributes, I updated them accordingly.
That is a valid point. The current solution was quickly added at the time and could be improved. I have created another code maintenance ticket for that. |
myabc
left a comment
There was a problem hiding this comment.
Did a very quick code review. This looks very good!
app/components/projects/settings/project_custom_field_sections/custom_field_row_component.rb
Show resolved
Hide resolved
frontend/src/stimulus/controllers/hover-card-trigger.controller.ts
Outdated
Show resolved
Hide resolved
frontend/src/stimulus/controllers/hover-card-trigger.controller.ts
Outdated
Show resolved
Hide resolved
frontend/src/stimulus/controllers/hover-card-trigger.controller.ts
Outdated
Show resolved
Hide resolved
frontend/src/stimulus/controllers/hover-card-trigger.controller.ts
Outdated
Show resolved
Hide resolved
78e3d31 to
f07767c
Compare
Ticket
https://community.openproject.org/wp/69596
Refactors hover cards to make use of HTML
templateelements. This conveys the correct semantic meaning, since an existing DOM element is copied and rendered in a different location -> exactly what a template should do.This elegant change additionally allowed us to streamline the behavior of the
HoverCardTriggerControllerfor both async and sync hover cards.Merge checklist