feat: add tooltip visibility control on scroll - #278
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
Migrates chart tooltips to Floating UI for viewport-aware positioning, scoped styling, and scroll dismissal.
Changes:
- Adds shared tooltip positioning and visibility hooks.
- Updates tooltip styling and chart integrations.
- Adds dependencies and reduces broad Lodash imports.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.changeset/green-schools-poke.md |
Documents the tooltip feature release. |
packages/dvz-ui/package.json |
Adds Floating UI and focused utility dependencies. |
pnpm-lock.yaml |
Locks new dependency versions. |
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx |
Implements tooltip positioning and visibility control. |
packages/dvz-ui/src/embeddable/common/ChartTooltip.jsx |
Applies shared positioning to additional charts. |
packages/dvz-ui/src/scss/themes/default/_charts.scss |
Adds viewport-safe tooltip styling. |
packages/dvz-ui/src/utils/semanticUtils.ts |
Replaces Lodash iteration with native APIs. |
packages/dvz-ui/src/embeddable/chart/Radar.jsx |
Removes an unused Lodash import. |
packages/dvz-ui/src/embeddable/big-filter/BigNumberGroup.jsx |
Uses the focused debounce package. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:248
onMouseOutbubbles when the pointer moves from this element onto any descendant. Since both rendering branches always add child elements, moving within the tooltip can setisHoveredto false and hide it.onMouseLeavealready handles actually leaving the tooltip without this bubbling behavior.
onMouseOut={hide}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:235
portalRootisnullon this first render, soFloatingPortalcreates its portal underdocument.body. Once Floating UI has created that portal node, changingrootdoes not relocate it, meaning the scoped.chart.tooltiprules can still be lost. Wait until the anchor callback has resolved the intended root before mounting the portal.
<FloatingPortal root={portalRoot}>
packages/dvz-ui/src/embeddable/common/ChartTooltip.jsx:69
- This portal is mounted while
portalRootis stillnull, causing Floating UI to create it underdocument.body; the later root update does not move an existing portal node. As a result, Sankey tooltips can still render outside the scoped theme ancestry. Mount the portal only after the anchor ref resolves its root.
<FloatingPortal root={portalRoot}>
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:250
onMouseOutalso fires when the pointer moves from this container into its inner<div>or any Markdown child. That normal movement hides the tooltip even though the pointer is still inside it.onMouseLeavealready handles exiting the whole tooltip, so remove the bubbling handler.
onMouseOut={hide}
.changeset/green-schools-poke.md:14
- The PR description says consumers must relocate their chart SCSS import and classifies this as a breaking change, but this changeset declares no breaking change and requests only a minor release. A required consumer integration change is semver-major for this 1.x package; update both the changeset bump and its migration note.
**Breaking changes:** None. Internal implementation detail; public API unchanged.
…e-tooltips-centered
commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (5)
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:89
- The global
pointerdownlistener also fires for presses inside the tooltip itself. Because it setshideOnScrollbefore the tooltip's touch/mouse handlers run, tooltip links or other content cannot be clicked/tapped andhandleTouchStartcannot make the tooltip visible again. The stated behavior is to dismiss on taps elsewhere, so the handler needs to ignore events whose target is inside the floating tooltip/reference.
window.addEventListener("scroll", hide, { capture: true, passive: true });
window.addEventListener("touchmove", hide, { capture: true, passive: true });
window.addEventListener("pointerdown", hide, { capture: true, passive: true });
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:237
- Hiding via CSS leaves the floating element mounted, so the
autoUpdate(..., { animationFrame: true })loop continues doing frame-by-frame measurements after scrolling. A sticky touch tooltip can remain mounted indefinitely, turning a dismissed tooltip into a permanent 60-fps layout cost. Detach the floating ref while hidden (or otherwise stopautoUpdate) and restore it when visible.
ref={floatingRef}
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:250
onMouseOutbubbles when the pointer moves between descendants, so a Markdown tooltip will hide as soon as the cursor enters a paragraph, link, or other child.onMouseLeavealready handles actually leaving the tooltip; remove the bubbling handler.
onMouseOut={hide}
.changeset/green-schools-poke.md:2
- The PR description marks this as a breaking change and explicitly requires consumers to move their chart SCSS import, but this changeset requests a minor release and says there are no breaking changes. Since the package is currently 1.9.7, either record a major bump and document the required migration here, or remove the migration requirement if compatibility is guaranteed.
"@devgateway/dvz-ui-react": minor
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:220
- Calling
preventDefault()for every tooltiptouchendsuppresses the synthesized click, so Markdown/raw-HTML links in the tooltip cannot be activated on touch devices. Avoid cancelling the default touch action; if emulated mouse events must be filtered, track the input modality and ignore only those mouse handlers instead.
const handleTouchEnd = (event) => {
// Prevent the emulated mouse events that follow a tap from immediately
// hiding the tooltip we just opened via touch.
event.preventDefault();
commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (7)
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:89
- This capture listener also handles pointer presses inside the portaled tooltip. On touch, it sets
isHiddenbefore the tooltip's ownonTouchStart, and that handler cannot clearisHidden, so tapping tooltip content immediately dismisses it. Ignore pointerdowns originating within the tooltip (while retaining scroll/touchmove dismissal).
window.addEventListener("pointerdown", hide, { capture: true, passive: true });
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:245
- After scroll dismissal this floating element remains mounted with only
visibility: hidden. BecauseuseTooltipPosition()enabledautoUpdate(..., { animationFrame: true }), the hidden/stuck Nivo tooltip continues measuring and updating every animation frame until Nivo eventually unmounts it. Unmount theFloatingPortalwheneverisVisibleis false (asChartTooltipalready does) or stop the auto-update subscription while hidden.
style={{
...floatingStyles,
visibility: isVisible ? "visible" : "hidden",
pointerEvents: isVisible ? "auto" : "none",
}}
packages/dvz-ui/src/embeddable/tooltip/index.tsx:66
- The new floating element only has
title-popup, while the removed Semantic UIPopupsupplied theui popup visible miniclasses that provide its border, shadow, typography, and z-index. The new SCSS rule only restores background and padding, so this tooltip loses most of its prior presentation. Preserve the base popup classes (or reproduce all of those styles explicitly).
className="title-popup"
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:250
mouseoutbubbles when the pointer moves between descendants, so markdown containing links, tables, or other nested elements hides the tooltip even though the pointer is still inside it.mouseleavealready handles actually exiting the tooltip; remove this handler.
onMouseOut={hide}
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:221
- Calling
preventDefault()on the tooltip container's bubblingtouchendalso cancels the synthesized click for links or other interactive HTML rendered by Markdown/rehypeRaw. Limit cancellation to touches directly on the container so child controls remain usable.
const handleTouchEnd = (event) => {
// Prevent the emulated mouse events that follow a tap from immediately
// hiding the tooltip we just opened via touch.
event.preventDefault();
.changeset/green-schools-poke.md:14
- The PR explicitly requires consumers to relocate chart SCSS imports and marks the change as breaking, but this changeset declares no breaking change and requests only a minor release. That would publish the integration break under a non-breaking version. Record the consumer action and use a major changeset.
"@devgateway/dvz-ui-react": minor
packages/dvz-ui/src/embeddable/tooltip/index.tsx:61
- The interaction props are attached to the
<span>, but keyboard focus is placed on Semantic UI's icon, which renders as an aria-hidden decorative element. Keyboard and screen-reader users therefore focus an unnamed element rather than the tooltip reference. Make the reference itself focusable and named.
<span ref={setReference} {...getReferenceProps()}>
<Icon name="question circle" tabIndex={0} />
6985ba7 to
193689c
Compare
commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (5)
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:89
- The capture listener hides the tooltip for every pointer press, including presses inside the portaled tooltip itself. Because
isHiddencan only reset whend/strchanges, tapping tooltip content (including links rendered from Markdown/HTML) dismisses it before activation andonTouchStartcannot make it visible again. Treat this as an outside press by excluding the floating element from the event path instead of hiding on allpointerdownevents.
window.addEventListener("scroll", hide, { capture: true, passive: true });
window.addEventListener("touchmove", hide, { capture: true, passive: true });
window.addEventListener("pointerdown", hide, { capture: true, passive: true });
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:250
onMouseOutfires when the pointer moves from this wrapper into any nested Markdown/HTML element because it bubbles. That immediately setsisHoveredto false and disables pointer events, so the tooltip disappears while the user moves within its content.onMouseLeavealready handles leaving the tooltip without firing for descendant transitions.
onMouseOut={hide}
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:220
- Calling
preventDefault()on a bubblingtouchendalso cancels the synthesized click/default action for anchors or other interactive HTML rendered inside the tooltip. This makes Markdown/HTML links unusable on touch devices; suppress the specific unwanted mouse transition without cancelling the content's default touch action.
const handleTouchEnd = (event) => {
// Prevent the emulated mouse events that follow a tap from immediately
// hiding the tooltip we just opened via touch.
event.preventDefault();
.changeset/green-schools-poke.md:14
- This conflicts with the PR's explicit downstream migration note and its breaking-change classification: requiring consumers to relocate their chart SCSS import is a breaking integration change even though the JavaScript API is unchanged. Either document that migration here and release accordingly, or remove the migration requirement if the scoped portal root makes it unnecessary.
**Breaking changes:** None. Internal implementation detail; public API unchanged.
.changeset/green-schools-poke.md:12
- The GroupedBars implementation only imports
formatContentfrom this module and does not renderChartTooltip, so it does not receive the new Floating UI positioning or scroll-dismissal behavior. Remove GroupedBars from this release-note claim unless its tooltip integration is also updated.
- Applied changes to Tooltip.jsx (Bar, Line, Pie charts) and ChartTooltip.jsx (Sankey, GroupedBars)
…e-tooltips-centered
commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:89
- This capture listener also runs when the user touches the tooltip itself, setting
isHiddento true beforehandleTouchStartruns. That handler only resetsisHovered, so the tooltip remains hidden despite the new touch handling. Ignore pointer downs originating inside the tooltip (the PR description only calls for dismissing taps elsewhere).
window.addEventListener("pointerdown", hide, { capture: true, passive: true });
packages/dvz-ui/src/embeddable/chart/Tooltip.jsx:248
mouseoutbubbles when the pointer moves between descendants. A Markdown/HTML tooltip with multiple elements therefore setsisHoveredto false while the pointer is still inside it, and the resultingvisibility: hiddenprevents another enter event from restoring it.onMouseLeavealready handles actually leaving the tooltip, so remove this handler.
onMouseOut={hide}
.changeset/green-schools-poke.md:14
- This contradicts the PR description, which marks the change as breaking and requires consumers to move their chart SCSS import. Reconcile the release note and changeset level with that migration requirement; if consumers really must update integration code, this 1.x package needs a major rather than minor changeset.
**Breaking changes:** None. Internal implementation detail; public API unchanged.
Description
Problem
Chart tooltips (Bar, Line, Pie, Sankey charts) had critical issues:
Solution
Migrated to @floating-ui/react with proper scroll detection and theme-scoped portaling:
Core Changes
useTooltipPosition()Hook (chart/Tooltip.jsx)useHideTooltipOnScroll()Benefits
NOTE
Projects will have to update their chart CSS where in the
scss/default/index.scssorscss/theme-name/index.scsswhere the chart SCSS file import should be moved outside of the
.edit-post-visual-editor, #root {}block since the tooltip is now rendered on a portal.Type of change
fix:)feat:)BREAKING CHANGE:)refactor:/chore:)docs:)Affected package(s)
@devgateway/dvz-ui-react@devgateway/wp-react-libexampleonly (no changeset needed)Checklist
pnpm changeset) for any change to a published packagepnpm buildpasses locallypnpm --filter @devgateway/* typecheckpasses