-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Refactor the tooltip positioning logic #1799
base: main
Are you sure you want to change the base?
Conversation
d640ee4
to
1f6d713
Compare
1f6d713
to
406caf4
Compare
@@ -0,0 +1,281 @@ | |||
import type {Story} from '@storybook/react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This story won't stay around, it's just for testing/development
yScale: yScale as ScaleLinear<number, number>, | ||
}); | ||
(event: MouseEvent | TouchEvent) => { | ||
const resetPosition = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the first part of the logic starts.
We watch mousemove
and if we're hovering over an element that provides us (at least) the activeIndex
, we will render the tooltip.
Most likely the element will also provide x
and y
, but at minimum the activeIndex
will render a tooltip for that index.
After we have x/y
positions, we check if the tooltip needs to be repositioned.
currentX, | ||
currentY, | ||
position, | ||
return repositionTooltip({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the second part of the logic.
Once we have the initial x/y
of the tooltip, we need to check if the position needs to be changed.
This can either be:
- The tooltip will render outside the bounds of the browser window.
- If the tooltip was repositioned back into the window, we check to see if it would be covering the data point and we try to reposition it again.
This logic is located in the useRepositonTooltip()
hook.
The reason we do this here and not in TooltipWrapper
is because we need access to the height & width of the tooltip. We don't have access to the actual sizing of the tooltip until we've rendered this component.
@@ -0,0 +1,337 @@ | |||
import type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For charts that always update their tooltip position (LineChart, DonutChart) the only repositioning we do it to keep the tooltip inside the bounds of the browser window and away from covering the cursor.
// outside the browser window, or cover the data series, | ||
// we want to reposition it again around the series bounds. | ||
if (isOutsideWindow && seriesBounds != null) { | ||
const positions: Position[] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For charts that don't update their position, we likely pass a seriesBounds
prop.
This prop is a set of x, y, height and width
values that we use to position the tooltip around.
For example, in a BarChart, if the tooltip has been repositioned into the seriesBounds
we will try and reposition the tooltip to the left of the seriesBounds
so that the tooltip does not cover the bars.
longestSeriesLength, | ||
}: Props) { | ||
return ( | ||
<Fragment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how we trigger the tooltip for a LineChart.
We render a rect
for each data point which acts as the hover zone. These rects are rendered as red boxes with blue outlines below.
Each rect
contains the data for it's activeIndex
as well as the expected x/y
for the tooltip.
When using a mouse, the x/y
values here are ignored because the tooltip follows the cursor, but are used when using the Tab
key to tab through the active points.
What does this implement/fix?
See if we can move away from positioning the tooltip based a bunch of math around the data and SVG points.
I think we should be able to use data attributes to get all the data we need to render and position the tooltips. We already have it available when we render the chart elements, why not just use that and skip all the
getXPosition
andgetAlteredX
functions we have for each chart.This should make adding tooltips to new charts way easier.
The basic idea is that the
TooltipWrapper
is always watching mouse movements for a chart. If it hovers over an element with a set ofdata-attributes
specific to rendering chartsx position, y position and activeIndex
we will render a tooltip for that specific point/index.Storybook link
https://6062ad4a2d14cd0021539c1b-oanoehqfpg.chromatic.com/?path=/story/polaris-viz-playground-tooltip-o-rama--tooltip-orama
Before merging
Check your changes on a variety of browsers and devices.
Update the Changelog's Unreleased section with your changes.
Update relevant documentation, tests, and Storybook.
Make sure you're exporting any new shared Components, Types and Utilities from the top level index file of the package