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

Overlay: Add min-width to Overlay container #5129

Merged
merged 20 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/forty-olives-lay.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

Overlay: Adds `min-width` to container to improve responsiveness
1 change: 1 addition & 0 deletions packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ export const AnchoredOverlay: React.FC<React.PropsWithChildren<AnchoredOverlayPr
left={position?.left || 0}
anchorSide={position?.anchorSide}
className={className}
preventOverflow={true}
{...overlayProps}
>
{children}
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/FeatureFlags/DefaultFeatureFlags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ export const DefaultFeatureFlags = FeatureFlagScope.create({
primer_react_css_modules_ga: false,
primer_react_action_list_item_as_button: false,
primer_react_select_panel_with_modern_action_list: false,
primer_react_overlay_overflow: false,
})
6 changes: 6 additions & 0 deletions packages/react/src/Overlay/Overlay.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@
"defaultValue": "",
"description": "If defined, Overlays will be rendered in the named portal. See also `Portal`."
},
{
"name": "preventOverflow",
"type": "boolean",
"defaultValue": "true",
"description": "Determines if the Overlay width should be adjusted responsively if `width` is set to either `auto`, `medium` or lower and there is not enough space to display the Overlay. If `preventOverflow` is set to `false`, the Overlay will be displayed at the maximum width that fits within the viewport."
},
{
"name": "sx",
"type": "SystemStyleObject"
Expand Down
88 changes: 52 additions & 36 deletions packages/react/src/Overlay/Overlay.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ export const OverlayOnTopOfOverlay = ({anchorSide, role}: OverlayProps) => {
role={role}
aria-modal={role === 'dialog' ? 'true' : undefined}
ref={primaryContainer}
preventOverflow={false}
>
<Button ref={secondaryButtonRef} onClick={() => setIsSecondaryOpen(!isSecondaryOpen)}>
open overlay
Expand All @@ -188,11 +189,12 @@ export const OverlayOnTopOfOverlay = ({anchorSide, role}: OverlayProps) => {
role={role}
aria-modal={role === 'dialog' ? 'true' : undefined}
ref={secondaryContainer}
preventOverflow={false}
>
<Box display="flex" flexDirection="column" p={2}>
<Text>Select an option!</Text>
<ActionMenu>
<ActionMenu.Button sx={{width: 200}}>{selectedItem}</ActionMenu.Button>
<ActionMenu.Button>{selectedItem}</ActionMenu.Button>
<ActionMenu.Overlay>
<ActionList selectionVariant="single">
{items.map(item => (
Expand Down Expand Up @@ -238,7 +240,7 @@ export const MemexNestedOverlays = ({role}: OverlayProps) => {
</ButtonGroup>
{overlayOpen && (
<Overlay
width="medium"
width="auto"
onEscape={() => setOverlayOpen(false)}
onClickOutside={() => setOverlayOpen(false)}
returnFocusRef={buttonRef}
Expand All @@ -248,6 +250,7 @@ export const MemexNestedOverlays = ({role}: OverlayProps) => {
role={role}
aria-modal={role === 'dialog' ? 'true' : undefined}
ref={containerRef}
preventOverflow={false}
>
<Box as="form" onSubmit={() => setOverlayOpen(false)} sx={{display: 'flex', flexDirection: 'column', py: 2}}>
<Box sx={{paddingX: 3, display: 'flex', alignItems: 'center', gap: 1}}>
Expand Down Expand Up @@ -327,6 +330,7 @@ export const NestedOverlays = ({role}: OverlayProps) => {
ignoreClickRefs={[buttonRef]}
top={100}
left={16}
preventOverflow={false}
ref={primaryContainer}
role={role}
aria-modal={role === 'dialog' ? 'true' : undefined}
Expand Down Expand Up @@ -434,17 +438,17 @@ export const MemexIssueOverlay = ({role}: OverlayProps) => {
{overlayOpen && (
<Overlay
height="auto"
width="large"
width="auto"
onEscape={() => setOverlayOpen(false)}
onClickOutside={() => setOverlayOpen(false)}
returnFocusRef={linkRef}
top={0}
left="calc(100vw - 480px)"
left="calc(100vw - 350px)"
role={role}
aria-modal={role === 'dialog' ? 'true' : undefined}
ref={containerRef}
>
<Box sx={{p: 4, height: '100vh'}}>
<Box sx={{p: 4, height: '100vh', width: '350px'}}>
<Box sx={{display: 'flex', alignItems: 'center', gap: 1, mb: 2}}>
<Label size="large">
<IssueDraftIcon /> Draft
Expand Down Expand Up @@ -488,7 +492,7 @@ export const MemexIssueOverlay = ({role}: OverlayProps) => {
aria-label="Change issue title"
sx={{
width: '100%',
fontSize: 4,
fontSize: 3,
color: 'fg.default',
p: 2,
textAlign: 'left',
Expand Down Expand Up @@ -559,25 +563,31 @@ export const PositionedOverlays = ({right, role}: OverlayProps) => {
>
<Box
sx={{
height: '100vh',
width: '500px',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
width: ['350px', '500px'],
}}
>
<IconButton
aria-label="Close"
onClick={closeOverlay}
icon={XIcon}
variant="invisible"
<Box
sx={{
position: 'absolute',
left: '5px',
top: '5px',
height: '100vh',
maxWidth: 'calc(-1rem + 100vw)',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
}}
/>
<Text>Look! left aligned</Text>
>
<IconButton
aria-label="Close"
onClick={closeOverlay}
icon={XIcon}
variant="invisible"
sx={{
position: 'absolute',
left: '5px',
top: '5px',
}}
/>
<Text>Look! left aligned</Text>
</Box>
</Box>
</Overlay>
) : (
Expand All @@ -597,25 +607,31 @@ export const PositionedOverlays = ({right, role}: OverlayProps) => {
>
<Box
sx={{
height: '100vh',
width: '500px',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
width: ['350px', '500px'],
}}
>
<IconButton
aria-label="Close"
onClick={closeOverlay}
icon={XIcon}
variant="invisible"
<Box
sx={{
position: 'absolute',
right: '5px',
top: '5px',
height: '100vh',
maxWidth: 'calc(-1rem + 100vw)',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
}}
/>
<Text>Look! right aligned</Text>
>
<IconButton
aria-label="Close"
onClick={closeOverlay}
icon={XIcon}
variant="invisible"
sx={{
position: 'absolute',
right: '5px',
top: '5px',
}}
/>
<Text>Look! right aligned</Text>
</Box>
</Box>
</Overlay>
)
Expand Down
57 changes: 51 additions & 6 deletions packages/react/src/Overlay/Overlay.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import theme from '../theme'
import BaseStyles from '../BaseStyles'
import {ThemeProvider} from '../ThemeProvider'
import {NestedOverlays, MemexNestedOverlays, MemexIssueOverlay, PositionedOverlays} from './Overlay.features.stories'
import {FeatureFlags} from '../FeatureFlags'

type TestComponentSettings = {
initialFocus?: 'button'
width?: 'small' | 'medium' | 'large' | 'auto' | 'xlarge' | 'xxlarge'
callback?: () => void
}
const TestComponent = ({initialFocus, callback}: TestComponentSettings) => {
const TestComponent = ({initialFocus, width = 'small', callback}: TestComponentSettings) => {
const [isOpen, setIsOpen] = useState(false)
const buttonRef = useRef<HTMLButtonElement>(null)
const confirmButtonRef = useRef<HTMLButtonElement>(null)
Expand All @@ -38,7 +40,7 @@ const TestComponent = ({initialFocus, callback}: TestComponentSettings) => {
ignoreClickRefs={[buttonRef]}
onEscape={closeOverlay}
onClickOutside={closeOverlay}
width="small"
width={width}
>
<Box display="flex" flexDirection="column" p={2}>
<Text>Are you sure?</Text>
Expand Down Expand Up @@ -154,16 +156,18 @@ describe('Overlay', () => {
const user = userEvent.setup()
const container = render(
<ThemeProvider>
<PositionedOverlays right />
<PositionedOverlays role="dialog" right />
</ThemeProvider>,
)

// open first menu
await user.click(container.getByText('Open right overlay'))
expect(container.getByText('Look! right aligned')).toBeInTheDocument()

const overlay = container.getByText('Look! right aligned').parentElement?.parentElement
const innerOverlay = container.getByText('Look! right aligned')
const overlay = container.getByRole('dialog')

expect(innerOverlay).toBeInTheDocument()
expect(overlay).toHaveStyle({position: 'fixed', right: 0})
expect(overlay).not.toHaveStyle({left: 0})

Expand All @@ -182,15 +186,18 @@ describe('Overlay', () => {
const user = userEvent.setup()
const container = render(
<ThemeProvider>
<PositionedOverlays />
<PositionedOverlays role="dialog" />
</ThemeProvider>,
)

// open first menu
await user.click(container.getByText('Open left overlay'))
expect(container.getByText('Look! left aligned')).toBeInTheDocument()

const overlay = container.getByText('Look! left aligned').parentElement?.parentElement
const innerOverlay = container.getByText('Look! left aligned')
const overlay = container.getByRole('dialog')

expect(innerOverlay).toBeInTheDocument()
expect(overlay).toHaveStyle({left: 0, position: 'absolute'})

spy.mockRestore()
Expand Down Expand Up @@ -282,4 +289,42 @@ describe('Overlay', () => {
// if stopPropagation worked, mockHandler would not have been called
expect(mockHandler).toHaveBeenCalledTimes(0)
})

it('should not have `data-reflow-container` if FF is not enabled', async () => {
const user = userEvent.setup()
const {getByRole} = render(<TestComponent />)

await user.click(getByRole('button', {name: 'open overlay'}))

const container = getByRole('none')
expect(container).not.toHaveAttribute('data-reflow-container')
})

it('should `data-reflow-container` if FF is enabled', async () => {
const user = userEvent.setup()
const {getByRole} = render(
<FeatureFlags flags={{primer_react_overlay_overflow: true}}>
<TestComponent />
</FeatureFlags>,
)

await user.click(getByRole('button', {name: 'open overlay'}))

const container = getByRole('none')
expect(container).toHaveAttribute('data-reflow-container')
})

it('should not have `data-reflow-container` if FF is enabled but the overlay is above `medium`', async () => {
const user = userEvent.setup()
const {getByRole} = render(
<FeatureFlags flags={{primer_react_overlay_overflow: true}}>
<TestComponent width="large" />
</FeatureFlags>,
)

await user.click(getByRole('button', {name: 'open overlay'}))

const container = getByRole('none')
expect(container).not.toHaveAttribute('data-reflow-container')
})
})
13 changes: 13 additions & 0 deletions packages/react/src/Overlay/Overlay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef'
import type {AnchorSide} from '@primer/behaviors'
import {useTheme} from '../ThemeProvider'
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
import {useFeatureFlag} from '../FeatureFlags'

type StyledOverlayProps = {
width?: keyof typeof widthMap
Expand Down Expand Up @@ -91,6 +92,10 @@ export const StyledOverlay = styled.div<StyledOverlayProps>`
outline: solid 1px transparent;
}

&[data-reflow-container='true'] {
max-width: calc(100vw - 2rem);
}

${sx};
`
type BaseOverlayProps = {
Expand All @@ -110,6 +115,7 @@ type BaseOverlayProps = {
preventFocusOnOpen?: boolean
role?: AriaRole
children?: React.ReactNode
preventOverflow?: boolean
}

type OwnOverlayProps = Merge<StyledOverlayProps, BaseOverlayProps>
Expand All @@ -133,6 +139,7 @@ type OwnOverlayProps = Merge<StyledOverlayProps, BaseOverlayProps>
* @param bottom Optional. Vertical bottom position of the overlay, relative to its closest positioned ancestor (often its `Portal`).
* @param position Optional. Sets how an element is positioned in a document. Defaults to `absolute` positioning.
* @param portalContainerName Optional. The name of the portal container to render the Overlay into.
* @param preventOverflow Optional. The Overlay width will be adjusted responsively if width is `auto`, `medium` or lower and there is not enough space to display the Overlay. If `preventOverflow` is `true`, the width of the `Overlay` will not be adjusted.
*/
const Overlay = React.forwardRef<HTMLDivElement, OwnOverlayProps>(
(
Expand All @@ -155,6 +162,7 @@ const Overlay = React.forwardRef<HTMLDivElement, OwnOverlayProps>(
preventFocusOnOpen,
position,
style: styleFromProps = {},
preventOverflow = true,
...rest
},
forwardedRef,
Expand Down Expand Up @@ -199,6 +207,10 @@ const Overlay = React.forwardRef<HTMLDivElement, OwnOverlayProps>(

// To be backwards compatible with the old Overlay, we need to set the left prop if x-position is not specified
const leftPosition: React.CSSProperties = left === undefined && right === undefined ? {left: 0} : {left}
const reflowSize = ['xsmall', 'small', 'medium', 'auto'].includes(width)

const enabled = useFeatureFlag('primer_react_overlay_overflow')
const overflow = enabled && reflowSize ? true : undefined

return (
<Portal containerName={portalContainerName}>
Expand All @@ -219,6 +231,7 @@ const Overlay = React.forwardRef<HTMLDivElement, OwnOverlayProps>(
...styleFromProps,
} as React.CSSProperties
}
data-reflow-container={overflow || (reflowSize && !preventOverflow) ? true : undefined}
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about this logic, wouldn't the second part be overriding the FF? or is that the intent here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this, I was thinking of adding the prop outside of the FF, but putting the opt-out factor inside the feature flag. So basically someone could use the prop today, but they'd have to pass it themselves.

if overflow is true, that means that the FF is enabled and a consumer would have to opt-out of it via preventOverflow=true. Otherwise, if the FF is not enabled, they must pass in preventOverflow="false" in order to get the reflow style.

Copy link
Member

Choose a reason for hiding this comment

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

ahh I see, thanks for explaining that!

/>
</Portal>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,10 @@ exports[`AnchoredOverlay should render consistently when open 1`] = `
outline: none;
}

.c3[data-reflow-container='true'] {
max-width: calc(100vw - 2rem);
}

@media (forced-colors:active) {
.c1:focus {
outline: solid 1px transparent;
Expand Down
Loading