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

Combobox: Respect targetRef at ComboboxPopover component #845

Closed

Conversation

vovakulikov
Copy link

@vovakulikov vovakulikov commented Oct 7, 2021

Context

Sometimes (pretty often) we have to change the target ref of the Combobox Popover element in case if we want to build something new with @reach/combobox. A good example of that can be a multi-Combobox with pills. I think this problem has been described already in this issue #543.

This PR fixes the problem when you can't pass a custom target ref to the <ComboboxPopover /> explicitly.

With input as a ref With combobox wrap as a ref
Screenshot 2021-10-08 at 00 03 27 image

This PR also updates tokens Combobox a little bit and shows new case usage with targetRef at <ComboboxPopover />

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 7, 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 7f223aa:

Sandbox Source
reach-ui-template Configuration

@indiesquidge
Copy link
Collaborator

@vovakulikov is there a reason for having a custom target ref other than for positioning the popover? If not, I've found that not using a portal, absolutely positioning the popover, and relatively positioning the element you want to align it with will achieve the behavior you're looking for as well.

If you still want to use the popover as a portal, as noted in #543, ComboboxPopover now accepts the position prop is of type Position and is passed on the underlying Popover. If you wanted to use your own target ref for the wrapping div for the position prop, you could write something like

import { positionMatchWidth } from "@reach/popover";
import { useRect } from "@reach/rect";

function Example() {
  const wrapperRef = React.useRef<HTMLDivElement | null>(null);
  const wrapperRect = useRect(wrapperRef);

  return (
    <div style={{ padding: 20, border: "1px solid tomato" }} ref={wrapperRef}>
      <Combobox id="holy-smokes" aria-label="choose a city">
        <ComboboxPopover position={(_, popoverRect) => positionMatchWidth(wrapperRect, popoverRect)} />
      </Combobox>
    </div>
  );
}

Which comes out looking something like

image

@vovakulikov
Copy link
Author

vovakulikov commented Oct 9, 2021

@indiesquidge thanks. I've missed that approach with position and useRect instead of passing target ref. You're right it's possible and probably it's more flexible rather than targetRef. I think it is worth adding this case to the Combobox description doc or at least to the storybook story. what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants