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

Tooltip: Add containerRef to Portal #848

Closed
wants to merge 4 commits into from

Conversation

gfohl
Copy link

@gfohl gfohl commented Oct 12, 2021

Add optional containerRef to Portal inside Tooltip.

Before

In full-screen, the reach-portal element existed but was not visible:

MicrosoftTeams-image (3)

After

By adding the target container to the Portal, the tooltip is now visible in full-screen

MicrosoftTeams-image (4)

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code (Compile and run).
  • Add or edit tests to reflect the change (Run with yarn test).
  • Add or edit Storybook examples to reflect the change (Run with yarn start).
  • Ensure formatting is consistent with the project's Prettier configuration.
  • Add documentation to support any new features.

This pull request:

  • Creates a new package
  • Fixes a bug in an existing package
  • Adds additional features/functionality to an existing package
  • Updates documentation or example code
  • Other

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 12, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7bb8abd:

Sandbox Source
reach-ui-template Configuration

@zpeterson
Copy link

This fixes an issue we're having where tooltips aren't visible when our video player is in full-screen mode. Would really appreciate having this change available!

@zpeterson
Copy link

Hi! Would love to get a review on this so we can ship soon!

@indiesquidge
Copy link
Collaborator

We understand why this change might be useful and necessary, but this library does not currently have full-time support, and we would like to consider the API a bit more before merging.

Keep in mind that you can always patch this behavior in the meantime in your own project with https://www.npmjs.com/package/patch-package

@indiesquidge
Copy link
Collaborator

indiesquidge commented Oct 26, 2021

An alternative to consider is updating Reach to include TooltipContent as a named export. (I'm not actually sure why this isn't done already considering we do export TooltipContentProps).

- export { MOUSE_REST_TIMEOUT, LEAVE_TIMEOUT, Tooltip, TooltipPopup, useTooltip };
+ export { MOUSE_REST_TIMEOUT, LEAVE_TIMEOUT, Tooltip, TooltipPopup, useTooltip, TooltipContent };

Doing this, we'd open up access to adjust the portal directly in a developer's custom tooltip however desired without adjusting the Tooltip's API.

Following the lead of the tooltip demos in the documentation using the lower level API, you could create something like this:

import * as React from "react";
import { useTooltip, TooltipContent } from "@reach/tooltip";
import type { TooltipProps } from "@reach/tooltip";
import Portal from "@reach/portal";
import "@reach/tooltip/styles.css";

interface CustomTooltipProps extends TooltipProps {
  containerRef?: React.RefObject<Node>;
}

function CustomTooltip({
  children,
  containerRef,
  ...props
}: CustomTooltipProps) {
  const [trigger, tooltip] = useTooltip();

  return (
    <React.Fragment>
      {React.cloneElement(children as any, trigger)}
      {tooltip.isVisible && (
        <Portal containerRef={containerRef}>
          <TooltipContent {...tooltip} {...props} />
        </Portal>
      )}
    </React.Fragment>
  );
}

@Warsaken
Copy link
Contributor

Warsaken commented Nov 8, 2021

I would like to see this added also to the "dialog" and "popover" (besides "tooltip") libraries in reach.

It was actually the original intent for containerRef when I introduced it in a previous PR (#756). I just left it separated to make reviewing the first PR easier.

@zpeterson
Copy link

Hey @indiesquidge! I wanted to check back in to determine the best path forward. It sounds like @Warsaken agrees with this update. Would it be possible to have this PR considered for approval?

@ccveer
Copy link

ccveer commented Feb 8, 2022

@indiesquidge, is there an eta for when this PR will get shipped? Would be a great update.

@chaance
Copy link
Member

chaance commented Jul 13, 2022

@gfohl Do you mind rebasing against the up-to-date branch? I've updated the repo structure a bit and moved examples into a separate playground package.

@chaance chaance added the Status: Awaiting Response Requested and awaiting a response from the issue creator label Jul 13, 2022
@chaance
Copy link
Member

chaance commented Jul 15, 2022

On second thought, I'm going to close this for the same reason I closed #901 (comment). Instead of a prop, I'd encourage composition. We already provide low-level access to the tooltip internals via useTooltip and Portal so you can create your own abstraction.

@chaance chaance closed this Jul 15, 2022
@chaance
Copy link
Member

chaance commented Jul 15, 2022

I have created an example of a custom tooltip implementation with a containerRef prop. https://github.com/reach/reach-ui/blob/dev/playground/stories/tooltip/custom-tooltip.example.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting Response Requested and awaiting a response from the issue creator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants