Skip to content
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

new(xychart): refine TooltipContext + Tooltip functionality #846

Merged
merged 14 commits into from
Oct 8, 2020

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Oct 6, 2020

TODO

🚀 Enhancements

@visx/tooltip
allow function signatures for useTooltip's updateTooltip + showTooltip

// before
showTooltip({ tooltipData, ... });

//after 
showTooltip(currentState => ({ _logic based on currentState_ }));

why? this mirrors how react setState works, and was needed to implement tooltip data merging properly in xychart/TooltipProvider. when TooltipProvider tries to merge tooltipData when the hook state is updated in quick succession, the hook-provided value may be stale.


@visx/xychart
Builds on #825 and adds more fully-featured Tooltip functionality:

  • TooltipProvider no longer simply returns the useTooltip hook state, but adds additional functionality so that TooltipContext.tooltipData now includes both the globally closest datum as well as the closest datum per-Series. Functionally this allows
    • for more comprehensive tooltip data to be rendered in the tooltip (i.e., data for all Series, see demo)
    • Tooltip to snap to data values instead of simply following the cursor
  • Tooltip now supports the following props
    • snapTooltipToDatumX, snapTooltipToDatumY (tooltip position snaps to data coord instead of mouse coord)
    • showVerticalCrosshair, showHorizontalCrosshair
    • showDatumCircle, showSeriesCircles
    • verticalCrosshairStyle, horizontalCrosshairStyle, circleStyle
  • adds a debounce to hideTooltip for better UX

@visx/demo

Updates the XYChart demo with all Tooltip controls

Also want to demo how you can do cool things like linked tooltips trivially with a shared EventEmitterContext which is TIGHT 🔥🔥

@kristw @hshoff @techniq

/** Whether to show a horizontal line at tooltip position. */
showHorizontalCrosshair?: boolean;
/** Whether to show a circle at the tooltip position for the (single) nearest Datum. */
showDatumCircle?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps name this showDatumMark or showDatumMarker ? In case you want to support other shapes (that is not circle) in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call. maybe I should make these use @visx/glyph 🤔 maybe I'll do that as a followup PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to glyph throughout with the future glyph support in mind

packages/visx-xychart/src/components/Tooltip.tsx Outdated Show resolved Hide resolved
packages/visx-xychart/src/components/Tooltip.tsx Outdated Show resolved Hide resolved
@@ -34,7 +39,7 @@ function LineSeries<XScale extends AxisScale, YScale extends AxisScale, Datum ex
(params: HandlerParams | undefined) => {
const { svgPoint } = params || {};
if (svgPoint && width && height && showTooltip) {
const datum = findNearestDatumXY({
const datum = (horizontal ? findNearestDatumX : findNearestDatumY)({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how to specify if the nearest XY is what developer want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are controlled with the horizontal prop right now. I removed xy because I realized that series should use the same logic for x vs y. Previously if bar used x and line used xy, you might have mismatched x values in the tooltip context (because voronoi don't map cleanly to x, it might not match the same x that the bar finds).

I agree this needs a better hook for dev experience or could be provided by DataContext at some point (you suggested this in the POC, maybe it'd be straightforward to infer from scale types 🤔 ).

@coveralls
Copy link

coveralls commented Oct 7, 2020

Pull Request Test Coverage Report for Build 130

  • 38 of 47 (80.85%) changed or added relevant lines in 6 files are covered.
  • 19 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.9%) to 57.448%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/visx-xychart/src/components/series/LineSeries.tsx 0 1 0.0%
packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx 0 2 0.0%
packages/visx-tooltip/src/hooks/useTooltip.ts 0 3 0.0%
packages/visx-xychart/src/components/Tooltip.tsx 31 34 91.18%
Files with Coverage Reduction New Missed Lines %
packages/visx-tooltip/src/hooks/useTooltipInPortal.tsx 1 0%
packages/visx-responsive/src/enhancers/withParentSize.tsx 5 58.06%
packages/visx-responsive/src/components/ParentSize.tsx 13 0%
Totals Coverage Status
Change from base Build 88: 0.9%
Covered Lines: 1285
Relevant Lines: 2172

💛 - Coveralls

* test(xychart): add more Tooltip tests

* test(xychart): add more TooltipProvider tests
@williaster williaster merged commit 5cd287f into master Oct 8, 2020
@williaster williaster deleted the chris--xychart-refined-tooltip branch October 8, 2020 00:17
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants