-
Notifications
You must be signed in to change notification settings - Fork 18
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
Added tooltip for TiledHeatmapMesh #1193
Conversation
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.
As we already discussed, flipped axes support is needed and transforms support would be nice to have but not needed for now.
To me in the end, the tooltip mesh will need to be handled outside of the TiledHeatmapMesh
because it should handle more than one mesh, but that's better to go step by step and it's fine for me like it is in this PR.
Agreed. But it means the variables computed in |
@@ -152,11 +172,7 @@ const Template: Story<TiledHeatmapStoryProps> = (args) => { | |||
<Zoom /> | |||
<SelectToZoom keepRatio modifierKey="Control" /> | |||
<ResetZoomButton /> | |||
<group | |||
scale={[abscissaConfig.flip ? -1 : 1, ordinateConfig.flip ? -1 : 1, 1]} | |||
> |
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.
The flip
is now handled at the layer level to avoid flipping the TiledTooltipMesh
@@ -46,9 +50,11 @@ function TiledHeatmapMesh(props: Props) { | |||
); | |||
const visibleBox = getObject3DVisibleBox(ndcToLocalMatrix); | |||
|
|||
const bounds = scaleToLayer(visibleBox, baseLayerSize, meshSize); | |||
const { xScale, yScale } = useLayerScales(baseLayerSize, meshSize); |
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.
Scales are now computed by a hook outside of scaleToLayer
(renamed scaleBox
) to be able to access the AxisContext
and take the flip in account
My bad, transforms (scale and position) also need to be supported, and flipped axes handled outside of the |
IMO, the most important is that the rendering of the tooltip content be the responsibility of the consumer, so there has to be a
Then, you can help consumers by providing a component to use inside In the long term, maybe #988 will bring out some refactoring opportunities to allow having a fully separate |
As the support of |
5b44cb2
to
6143e56
Compare
6143e56
to
f5198ad
Compare
Now handling transforms. I may revert the refactoring of |
/> | ||
))} | ||
</group> | ||
{showTooltip && <TiledTooltipMesh size={meshSize} />} |
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 to make the size of TooltipMesh
match the size of the TiledHeatmap
return ( | ||
<TooltipMesh | ||
size={size} | ||
renderTooltip={() => { |
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.
renderTooltip
could be exposed
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.
Yeah this is great for this PR. It can be passed down as prop in a separate PR.
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.
I reckon that the tooltip store is taking us really close to a shared solution for both the heatmap and the tiled heatmap, where the TooltipMesh
would remain outside of/decoupled from the (Tiled)HeatmapMesh
component. This has great potential: it could allow, for instance, consumers of the lib replace the tooltip with a status bar, a bit like on vector graphics software:
return ( | ||
<TooltipMesh | ||
size={size} | ||
renderTooltip={() => { |
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.
Yeah this is great for this PR. It can be passed down as prop in a separate PR.
No need, it's clear what's what, and the refactoring is nice 👍 |
f5198ad
to
c6044a6
Compare
c6044a6
to
228e41c
Compare
Related to #1177
Tried an analytical method as using the raycaster was not convienient at all. Indeed, as the whole layer group is intersected by the ray, we still need to filter manually the tiles. Perhaps I missed something so I could revisit this later.
The tooltip is not working properly for two cases:
I am fairly confident I can fix it for flipped axes but we can already discuss the current implementation.