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(vx-tooltip): add useTooltipInPortal #756

Merged
merged 9 commits into from
Jun 25, 2020

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Jun 19, 2020

💥 Breaking Changes

🚀 Enhancements

📝 Documentation

This PR is into #755 and

  • adds a new userTooltipInPortal hook to addresses the issue of Portal not being easy to use by lib consumers. Notes:
    • this adds a new react-use-measure dependency for the robust bounds logic
    • react-use-measure relies on ResizeObserver. rather than adding the polyfill here like we do in @vx/responsive, I'm opting to just expose the useMeasure polyfill injection system. So, users can add it themselves to window or pass it in directly to the hook. I've noted this in the Readme.
  • updates the tooltip demo added in new(vx-tooltip): add Portal + demo #755 to use this hook instead of Portal + react-use-measure
  • updates the vx-tooltip Readme to include useTooltipInPortal and Portal
  • updates the barstack example to use useTooltipInPortal as a validation point within svgs
  • adds offsetLeft/Top to TooltipProps, making TooltipProps === TooltipWithBoundsProps. This simplifies type checking in the new useTooltipInPortal, and is a breaking change for Tooltip as it will add 10px of padding to the Tooltip left/top.

Using userTooltipInPortal
image

TODO

  • remove react-use-measure dep in vx-demo

@hshoff @kristw @ktmud @techniq

<TooltipWrapper>
<TooltipComponent
key={Math.random()} // needed for bounds to update correctly
left={(tooltipLeft ?? 0) + (detectBounds ? 0 : 10)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noting that we got to remove this conditional 10 by making the Tooltip APIs consistent.

@techniq
Copy link
Collaborator

techniq commented Jun 20, 2020

LGTM.

I recently started using react-use-measure to calculate some offsets when using position: sticky for table headers. I also created a Measure component within our application to simplify usage, as well as handle scrolling offsets (viewport prop).

Should we consider using react-use-measure within ParentSize as well (I had considered using this Measure component in place of ParentSize within our components but hadn't yet.

I know we just added support for a leading debounce trigger and currently react-use-measure does not have this, but it looks like they use debounce and it has support for firing on the leading edge (immediate)

import React from 'react';
import useMeasure, { Options, RectReadOnly } from 'react-use-measure';

type MeasureProps = Options & {
  children: (bounds: RectReadOnly) => React.ReactNode;
  viewport?: boolean;
};

function Measure(props: MeasureProps) {
  const { children, viewport, ...options } = props;

  let [ref, bounds] = useMeasure({ debounce: 300, ...options });

  if (viewport) {
    // See note about scrolling position: https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect
    bounds = {
      ...bounds,
      left: bounds.left + window.scrollX,
      top: bounds.top + window.scrollY,
      x: bounds.x + window.scrollX,
      y: bounds.y + window.scrollY
    };
  }

  return <div ref={ref}>{children(bounds)}</div>;
}

export default Measure;

@williaster
Copy link
Collaborator Author

yeah I think updating ParentSize to use react-use-measure could be a good call, it would enable polyfill injection + fix scroll bounding box issues which we don't currently correct for 👍

@williaster
Copy link
Collaborator Author

williaster commented Jun 25, 2020

gonna merge this into the parent branch (not master)

@williaster williaster merged commit e441821 into chris--tooltip-portal Jun 25, 2020
@williaster williaster deleted the chris--useTooltipInPortal branch June 25, 2020 20:00
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.

None yet

3 participants