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

Export HiddenContext from the utils in react-aria-components #6459

Closed
wants to merge 3 commits into from

Conversation

thexpand
Copy link

@thexpand thexpand commented May 28, 2024

This will give user land components the opportunity to use the context to replace components like the Popover in the Select component without breaking the functionality of the Select.

Also, export the forwardRefType so that we can build components just like they are built inside react-aria-components.

Closes #6453

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • N/A Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • N/A Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. Render a JSX tree using react-aria-components:
<Select>
  <Label>Favorite Animal</Label>
  <Button>
    <SelectValue />
    <span aria-hidden="true"></span>
  </Button>
  <Popover>
    <ListBox>
      <ListBoxItem>Aardvark</ListBoxItem>
      <ListBoxItem>Cat</ListBoxItem>
      <ListBoxItem>Dog</ListBoxItem>
      <ListBoxItem>Kangaroo</ListBoxItem>
      <ListBoxItem>Panda</ListBoxItem>
      <ListBoxItem>Snake</ListBoxItem>
    </ListBox>
  </Popover>
</Select>
  1. Create a new component to replace the Popover:
function PopoverReplacement(props: {  }, ref: ForwardedRef<HTMLElement>) {
  const state = useContext(OverlayTriggerStateContext);
  const isHidden = useContext(HiddenContext);

  if (isHidden) {
    return <>{props.children}</>;
  }

  if (state && !state.isOpen) {
    return null;
  }

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

const _PopoverReplacement = /*#__PURE__*/ (forwardRef as forwardRefType)(PopoverReplacement);
export {_PopoverReplacement as PopoverReplacement};
  1. Render the PopoverReplacement instead of the Popover in the JSX created in point 1:
<Select>
  <Label>Favorite Animal</Label>
  <Button>
    <SelectValue />
    <span aria-hidden="true"></span>
  </Button>
  <PopoverReplacement>
    <ListBox>
      <ListBoxItem>Aardvark</ListBoxItem>
      <ListBoxItem>Cat</ListBoxItem>
      <ListBoxItem>Dog</ListBoxItem>
      <ListBoxItem>Kangaroo</ListBoxItem>
      <ListBoxItem>Panda</ListBoxItem>
      <ListBoxItem>Snake</ListBoxItem>
    </ListBox>
  </PopoverReplacement >
</Select>

The select should continue to work as usual and have the options pre-populated, even when the popover replacement is not visible yet, e.g. during SSR.

🧢 Your Project:

react-aria-components

@thexpand
Copy link
Author

Looks like there are failing tests but they are not triggered because of the changes in this PR, but by something else.

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM

Question for the team: are we ok exporting forwardRefType?

@devongovett
Copy link
Member

I'm not sure about making our hacks into public APIs. Let's think about this a bit more. 😅

@thexpand
Copy link
Author

thexpand commented May 28, 2024

@devongovett Exporting forwardRefType is not that important in this instance, to be honest. I mean, probably anyone who uses generics in user land code and uses forward ref on the same components would need already has a similar type to battle TypeScript. We can remove the export of this "hack".

@sadeghbarati

This comment was marked as off-topic.

@thexpand
Copy link
Author

thexpand commented Jun 3, 2024

What would be the reason to do so, @sadeghbarati ? My particular use case is to introduce a custom popover component (in place of the one provided by React Aria).

@sadeghbarati

This comment was marked as off-topic.

@snowystinger
Copy link
Member

snowystinger commented Jun 3, 2024

This is a separate issue, let's keep the discussion there.

@devongovett
Copy link
Member

devongovett commented Jun 4, 2024

My concern is that it feels very "implementation-detail"-y. I think we want to allow creating custom popovers to work with select/combobox, but as it is, this seems quite difficult to document, and hard for us to maintain over time if we end up changing the implementation. Seems like we need a more official API, but I'm not sure what that would be yet.

In the meantime, does it work if you use display: none when the popover is closed instead of removing it from the DOM by returning null? This would mean that the popover/listbox would always mount, but the browser wouldn't render it. Not totally ideal because of the extra DOM nodes, but does it work as a temporary solution?

@thexpand
Copy link
Author

thexpand commented Jun 4, 2024

@devongovett Using simply a display: none won't work, because I need to render a ListBox inside my popover as well. Doing so will render the options twice.

I think we can leave it at just exporting the HiddenContext, as we have other contexts exported as well.

What do you think?

@thexpand thexpand force-pushed the patch-1 branch 2 times, most recently from adcd5b9 to 7bbe0bc Compare June 4, 2024 22:20
thexpand and others added 3 commits June 17, 2024 00:20
This will give user land components the opportunity to use the context to replace components like the Popover in the Select component without breaking the functionality of the Select.
…ents that are like the ones in `react-aria-components`
@devongovett
Copy link
Member

resolved by #6640

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.

HiddenContext not exported, can't build my own Popover within a Select field
5 participants