react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Export `HiddenContext` from the utils in `react-aria-components`

Open thexpand opened this issue 1 year ago โ€ข 9 comments

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:

  • [x] 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).~~
  • [x] Filled out test instructions.
  • N/A ~~Updated documentation (if it already exists for this component).~~
  • [x] 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 avatar May 28 '24 11:05 thexpand

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

thexpand avatar May 28 '24 12:05 thexpand

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

devongovett avatar May 28 '24 15:05 devongovett

@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".

thexpand avatar May 28 '24 19:05 thexpand

Please also export other useful functions/components like

  • useCollectionDocument
  • CollectionDocumentContext
  • Hidden
  • removeDataAttributes
  • useRenderProps
  • RenderProps and forwardRefType and RACValidation (types)

sadeghbarati avatar May 31 '24 19:05 sadeghbarati

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).

thexpand avatar Jun 03 '24 19:06 thexpand

What would be the reason to do so

@thexpand https://github.com/adobe/react-spectrum/issues/2140#issuecomment-2083302614

sadeghbarati avatar Jun 03 '24 23:06 sadeghbarati

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

snowystinger avatar Jun 03 '24 23:06 snowystinger

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?

devongovett avatar Jun 04 '24 01:06 devongovett

@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 avatar Jun 04 '24 06:06 thexpand

resolved by #6640

devongovett avatar Jul 12 '24 22:07 devongovett