chore: Fixes mixed charts popover behaviour for React 18 #3851
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3851 +/- ##
==========================================
+ Coverage 97.14% 97.20% +0.05%
==========================================
Files 844 844
Lines 24515 24555 +40
Branches 8645 8659 +14
==========================================
+ Hits 23815 23868 +53
+ Misses 693 639 -54
- Partials 7 48 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pan-kot
changed the title
| if (this.currentIndex) { | ||
| await this.click(this.wrapper.findDetailPopover().findDismissButton().toSelector()); | ||
| // Wait for popover dismiss reopen delay. | ||
| await this.pause(50); |
There was a problem hiding this comment.
The new implementation prevents the popover to be shown immediately after dismiss using a small timeout, which needs to be considered also in tests.
| // The delay prevents re-opening popover immediately upon dismissing, | ||
| // so that the popover actually closes. It can be reopened with the next | ||
| // hover or focus event that occurs after the delay. | ||
| const REOPEN_DELAY_MS = 50; |
There was a problem hiding this comment.
The popover should not be reopen immediately upon dismissal - that is an existing requirement. That was ensured by checking the highlighted series index/id: if same, the popover will not reopen. The new implementation achieves the same with a small timeout, yet now it is also possible to re-open the popover shortly after it is dismissed by hovering over the same series the popover was open for before.
| scaledX: point.x, | ||
| label: point.datum?.x ?? null, | ||
| }); | ||
| showPopover(); |
There was a problem hiding this comment.
We now show the popover the moment a point or series is highlighted, without relying on the dependencies change.
| clearAllHighlights(); | ||
| dismissPopover(); | ||
| }, [dismissPopover, clearAllHighlights]); | ||
| hidePopover(); |
There was a problem hiding this comment.
Hiding the popover (e.g. when the cursor leaves a series) is not the same as dismissing it - we should not prevent its reopen in that case.
| if (highlightedX !== null || highlightedPoint !== null) { | ||
| showPopover(); | ||
| } | ||
| }, [highlightedX, highlightedPoint, showPopover]); |
There was a problem hiding this comment.
If highlightedX was 1 (showing the popover for the 1st group) and then it changes as 1 -> null -> 1 in a quick succession, that intermediate transition to null is ignored by React 18. As result, the effect callback is not fired, thus not showing the popover.
| expectValues([47, 7, 8]); | ||
| }); | ||
|
|
||
| test('opens popover for each series (line)', () => { |
There was a problem hiding this comment.
This test is needed to increase test coverage of the mixed chart container
Description
The issue was observed when we run integ tests with React 18: #3829
With React 18 (strict mode or not) the mixed charts popover behaves differently: when the cursor moves from popover content to a bar series it was open for, the popover does not reopen: the user has to move the cursor outside the bar, and then back inside.
That happens because the popover state is controlled with a useLayoutEffect, and it depends on a series of state changes:
See how the popover re-opens when the cursor is moved slowly, but does not reopen when it is moved fast:
Screen.Recording.2025-09-10.at.09.41.45.mov
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.