Skip to content

Conversation

snowystinger
Copy link
Member

Closes

✅ Pull Request Checklist:

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

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Jan 20, 2024

ktabors
ktabors previously approved these changes Jan 26, 2024
Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

I know we have tests, but I wouldn't mind having a story for this too.

expect(getByRole('menu').closest('[data-testid="custom-container"]')).toBe(getByTestId('custom-container'));
});
it('should render the menu tray in the portal container', async () => {
windowSpy.mockImplementation(() => 700);
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this test made me realize we didn't have a test for Menu loading as a tray on mobile. Submenu does have one. :)

...tooltipProps
}}>
<Overlay isOpen={state.isOpen} nodeRef={overlayRef}>
<Overlay isOpen={state.isOpen} nodeRef={overlayRef} container={UNSTABLE_portalContainer}>
Copy link
Member

Choose a reason for hiding this comment

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

Was this part of the request? It previously didn't support container. I'm trying to think of the case for this because Tooltip is usually the text label for an icon button.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the question

Copy link
Member

Choose a reason for hiding this comment

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

The container prop here is on RSP Overlay which we don't expose so it should be fine (said container gets set as the portal container internally). Users can pass UNSTABLE_portalContainer to TooltipTrigger though which falls in line with the intended change

@snowystinger
Copy link
Member Author

I need to add DialogContainer

@rspbot
Copy link

rspbot commented Jan 26, 2024

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

@rspbot
Copy link

rspbot commented Feb 2, 2024

@rspbot
Copy link

rspbot commented Feb 2, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-spectrum/dialog

DialogContainer

 SpectrumDialogContainerProps {
+  UNSTABLE_portalContainer?: Element = document.body
   children: ReactNode
   isDismissable?: boolean
   isKeyboardDismissDisabled?: boolean
   onDismiss: () => void
 }
 

useDialogContainer

 SpectrumDialogTriggerProps {
+  UNSTABLE_portalContainer?: Element = document.body
   children: [ReactElement, SpectrumDialogClose | ReactElement]
   hideArrow?: boolean
   isDismissable?: boolean
   isKeyboardDismissDisabled?: boolean
   targetRef?: RefObject<HTMLElement>
   type?: 'modal' | 'popover' | 'tray' | 'fullscreen' | 'fullscreenTakeover' = 'modal'
 }
 

@react-spectrum/menu

ContextualHelpTrigger

 SpectrumActionMenuProps<T> {
+  UNSTABLE_portalContainer?: HTMLElement = document.body
   align?: Alignment = 'start'
   autoFocus?: boolean
   closeOnSelect?: boolean = true
   direction?: 'bottom' | 'top' | 'left' | 'right' | 'start' | 'end' = 'bottom'
   isQuiet?: boolean
   onAction?: (Key) => void
   shouldFlip?: boolean = true
   trigger?: MenuTriggerType = 'press'
 }
 

Section

 SpectrumMenuTriggerProps {
+  UNSTABLE_portalContainer?: HTMLElement = document.body
   align?: Alignment = 'start'
   children: Array<ReactElement>
   closeOnSelect?: boolean = true
   direction?: 'bottom' | 'top' | 'left' | 'right' | 'start' | 'end' = 'bottom'
   trigger?: MenuTriggerType = 'press'
 }
 

@react-spectrum/overlays

Modal

 Tray {
   children: ReactNode
+  container?: Element
   isFixedHeight?: boolean
   state: OverlayTriggerState
 }

@react-spectrum/tooltip

TooltipTrigger

 SpectrumTooltipTriggerProps {
+  UNSTABLE_portalContainer?: Element = document.body
   children: [ReactElement, ReactElement]
   delay?: number = 1500
   isDisabled?: boolean
   offset?: number = 7
 }
 

@LFDanLu LFDanLu merged commit 7c1da9f into main Feb 2, 2024
@LFDanLu LFDanLu deleted the expose-unstable-portalcontainer branch February 2, 2024 20:59
snowystinger added a commit that referenced this pull request Feb 5, 2024
snowystinger added a commit that referenced this pull request Feb 5, 2024
This reverts commit Expose UNSTABLE container for RSP overlays (#5723).
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.

5 participants