Dev 660 fix tooltips behavior on mobiletablet screen sizes#223
Conversation
📝 WalkthroughWalkthroughThe pull request implements scroll-based tooltip dismissal functionality by refactoring the UI tooltip service to register/unregister scroll listeners during the tooltip lifecycle, centralizing DOM style mutations into helper methods, and adding comprehensive unit and end-to-end tests for the new behavior. Changes
Sequence DiagramsequenceDiagram
participant User as User/<br/>Page
participant Service as UI Tooltip<br/>Service
participant DOM as DOM/Tooltip<br/>Element
participant Events as Window/<br/>Document
User->>Service: show(tooltip, anchor)
activate Service
Note over Service: Unregister existing<br/>listeners (if any)
Service->>DOM: prepareTooltipForPositioning()
Service->>DOM: Update styles
Service->>Service: Set activeTooltipElement<br/>activeAnchorElement
Service->>Events: registerDismissListeners()
Service->>Events: Attach scroll listeners<br/>(capture: true)
deactivate Service
DOM->>DOM: display: block<br/>opacity: 1
User->>Events: Scroll window/element
Events->>Service: scroll event (capture)
activate Service
Service->>Service: hideActiveTooltip()
Service->>Service: hide(activeTooltipElement)
Service->>DOM: resetTooltipStyles()
Service->>DOM: Update styles
Service->>Events: unregisterDismissListeners()
Service->>Events: Remove scroll listeners
deactivate Service
DOM->>DOM: display: none<br/>opacity: 0
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
daroczig
temporarily deployed
to
dev-660-fix-tooltips-behavior-on-mobiletablet-screen-sizes - sc-www PR #223
Render
Destroyed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/services/ui-tooltip.service.ts (1)
14-16: Considerpassive: truefor the scroll listener.Since this PR targets mobile/tablet, adding
passive: truelets the browser scroll without waiting on the listener and avoids any chance of triggering the Chrome "non-passive scroll listener" warning. The handler only callshide()(nopreventDefault), so making it passive is safe.♻️ Proposed refactor
- private readonly dismissListenerOptions: AddEventListenerOptions = { - capture: true, - }; + private readonly dismissListenerOptions: AddEventListenerOptions = { + capture: true, + passive: true, + };Also applies to: 217-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/services/ui-tooltip.service.ts` around lines 14 - 16, The dismissListenerOptions object used for scroll/dismiss event listeners (private readonly dismissListenerOptions: AddEventListenerOptions) should include passive: true since the scroll handler only calls hide() and does not call preventDefault; update that options object to add passive: true so the browser can optimize scrolling and avoid non-passive listener warnings, and apply the same change to the other scroll/dismiss listener options used later in the file (the listener options referenced around the scroll/hide handlers and where hide() is attached) so all scroll listeners are passive.cypress/e2e/servers_compare.cy.ts (1)
10-24: Consider using Cypress'strigger()method for cleaner event simulation.The component correctly binds
(mouseenter)directly on thelucide-icon[name="info"]elements throughout the template, so the synthetic dispatch will work. However,cy.get('#main-table tr.rows-to-hide-for-test lucide-icon[name="info"]').first().trigger('mouseenter')is the idiomatic Cypress approach and handles event simulation more robustly than manualdispatchEvent().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/servers_compare.cy.ts` around lines 10 - 24, The custom MouseEvent dispatch in showCompareTooltip() should be replaced with Cypress's built-in trigger for cleaner, more robust event simulation; locate showCompareTooltip and change the body that queries '#main-table tr.rows-to-hide-for-test lucide-icon[name="info"]' and first() so it calls .trigger('mouseenter') (optionally with { force: true } if needed) instead of manually creating and dispatching a new win.MouseEvent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cypress/e2e/servers_compare.cy.ts`:
- Around line 10-24: The custom MouseEvent dispatch in showCompareTooltip()
should be replaced with Cypress's built-in trigger for cleaner, more robust
event simulation; locate showCompareTooltip and change the body that queries
'#main-table tr.rows-to-hide-for-test lucide-icon[name="info"]' and first() so
it calls .trigger('mouseenter') (optionally with { force: true } if needed)
instead of manually creating and dispatching a new win.MouseEvent.
In `@src/app/services/ui-tooltip.service.ts`:
- Around line 14-16: The dismissListenerOptions object used for scroll/dismiss
event listeners (private readonly dismissListenerOptions:
AddEventListenerOptions) should include passive: true since the scroll handler
only calls hide() and does not call preventDefault; update that options object
to add passive: true so the browser can optimize scrolling and avoid non-passive
listener warnings, and apply the same change to the other scroll/dismiss
listener options used later in the file (the listener options referenced around
the scroll/hide handlers and where hide() is attached) so all scroll listeners
are passive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 208f51c5-591d-4387-a7db-793e4cfdc326
📒 Files selected for processing (3)
cypress/e2e/servers_compare.cy.tssrc/app/services/ui-tooltip.service.spec.tssrc/app/services/ui-tooltip.service.ts
|
what about this suggestion? 
|
I'm mostly don't bother myself with nitpick comments because those are rarely critical change suggestions. The current code is fine as-is, and that |
daroczig
left a comment
daroczig
left a comment
There was a problem hiding this comment.
sounds good, I trust your related decision 👍 approved 
on the other hand, we might need to still look into these at some point (independent of this update) to reduce the related warnings in the dev console:
might improve our lighthouse score :)
I'll create a ticket in the backlog to collect all warnings and solve them when we'll have time for it! 🦺 |
martonszak
deleted the
dev-660-fix-tooltips-behavior-on-mobiletablet-screen-sizes
branch
2 participants
Overview
Required checks
Mark all tasks that were completed with a checkmark. Unrelated tasks can be left unchecked.
Required checks before asking for human review:
mainFurther required checks after all other major items in this list are completed:
Summary by CodeRabbit