Skip to content

Commit 4fd4f6d

Browse files
gggritsogggritso
authored
fix: Remove open prop of SlideOverPanel (#104627)
Right now, `SlideOverPanel` can be opened/closed by passing `<SlideOverPanel open={isPanelOpen}` or by conditionally rendering it with `isPanelOpen && <SlideOverPanel`. This is problematic because when there are two ways to open/close something, it means there are multiple ways we need to account for when thinking about animations and deferring rendering the contents. This PR removes the `open` prop. `SlideOverPanel` now _must_ be rendered conditionally with `&&` in JSX. This required an update to how the contents are deferred on mount, but otherwise behaviour is preserved. I was pushed towards the `isOpen &&` API due to a practical problem in Dashboards, but I also think it's a reasonable choice overall. ## Rationale Consider: ``` <Container> <SlideOverPanel ... <PreviewWidget /... </Container> ``` Imagine that the container has a panel, and a preview which both take up half the screen and slide in from opposite sides. The container takes up the full screen, and is shown over some background content. It's offset against the main sidebar. - the container _must_ exist to account for the sidebar, and to correctly split space between the panel and preview - the container must take up the whole screen, which means it prevents clicks on content underneath - I need to hide/show the container, panel, preview when users click on buttons in the UI - the panel and preview both have a slight animation to make it less jarring when they open-close ``` +-------------------------+ | +---------++---------+ | | | || | | | | || | | | | Panel || Preview | | | | || | | | | || | | | | || | | | +---------++---------+ | | | | Container | | | +-------------------------+ ``` There are two major problems. 1. How should I go about hiding/showing the container? The container blocks the content underneath. When the panel and the preview close, how do I hide the container? I could either make it "invisible" by turning off opacity and pointer events, or use `display: none`. The former is messy, and error prone. It's easy to accidentally leave this in, and cause issues with clicks. The latter is difficult, how do I coordinate setting `display: none` vs. the panel and preview when they slide? This is the case in the Widget Builder. 2. What if I need to remount the `Container` for some reason? e.g., the container might have some kind of context, or something else that needs to be remounted or re-rendered that might cause a re-mount of the panel. In that case, deferring the content of the panel won't work, since it's relying on the `open` prop state _changing_, but on initial mount there is no change. This is also the case in the Widget Builder. It's not possible to have a deferral mechanism that works both on mount and on prop change! If the API is `isOpen &&` these problems are solved. I can just mount/unmount the entire tree, and everything works as expected. Since there is only one way to show/hide the panel, I can use one deferral method, and I don't need explanations in the doc for how to handle animations and deferral. In short, controlling mounting/unmounting is a lot easier than controlling an `open` prop because I don't have to worry about keeping surrounding components in the DOM just to make animations and deferred rendering work.
1 parent 5d638b1 commit 4fd4f6d

File tree

6 files changed

+151
-136
lines changed

6 files changed

+151
-136
lines changed

‎static/app/components/core/slideOverPanel/slideOverPanel.mdx‎

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,18 +28,19 @@ The `SlideOverPanel` component is a panel that can appear on the right, left, or
2828
## Basic Usage
2929

3030
```jsx
31-
<Button onClick={() => setIsPanelOpen(true)}>Open Panel</Button>
32-
33-
<SlideOverPanel open={isPanelOpen} position="right">
34-
<Container border="primary" height="100%" padding="md">
35-
<Button onClick={() => setIsPanelOpen(false)}>Close Panel</Button>
36-
</Container>
37-
</SlideOverPanel>
31+
<Button onClick={() => setIsPanelOpen(true)}>Open Panel</Button>;
32+
33+
{
34+
isPanelOpen && (
35+
<SlideOverPanel position="right">
36+
<Container border="primary" height="100%" padding="md">
37+
<Button onClick={() => setIsPanelOpen(false)}>Close Panel</Button>
38+
</Container>
39+
</SlideOverPanel>
40+
);
41+
}
3842
```
3943

40-
> [!NOTE]
41-
> It's also possible to conditionally render the panel (e.g., `{isPanelOpen && <SlideOverPanel ...`). We do not recommend this approach. If you do this, the panel will not defer rendering its contents (see below).
42-
4344
## Skeleton UI
4445

4546
`SlideOverPanel` defers rendering its contents. When the panel is rendered, it will open immediately, and its contents will render in a subsequent pass. This means the panel can respond immediately to user input even if the contents of the panel are expensive to render. This is much better UX, and improves INP.
@@ -59,15 +60,37 @@ To enable a loading skeleton, pass a render prop as the children to `SlideOverPa
5960
```jsx
6061
<Button onClick={() => setIsPanelOpen(true)}>Open Panel</Button>
6162

62-
<SlideOverPanel open={isPanelOpen} ePosition="right">
63-
{({isOpening}) => isOpening ? <Placeholder /> :
64-
<Container border="primary" height="100%" padding="md">
65-
<Button onClick={() => setIsPanelOpen(false)}>Close Panel</Button>
66-
</Container>
67-
}
68-
</SlideOverPanel>
63+
{isPanelOpen && (
64+
<SlideOverPanel position="right">
65+
{(options: {isOpening: boolean}) => {
66+
return options.isOpening ? (
67+
<SkeletonPanelContents onClick={closePanel} />
68+
) : (
69+
<PanelContents onClick={closePanel} />
70+
);
71+
}}
72+
</SlideOverPanel>
73+
)}
6974
```
7075

7176
## Animating Panel Close
7277

7378
Generally, we recommend not animating the panel on close, and simply removing it from the UI. In cases where you need to animate closing it (e.g., if it's coordinated with another animation), please wrap the panel in `<AnimatePresence>`.
79+
80+
```jsx
81+
<Button onClick={() => setIsPanelOpen(true)}>Open Panel</Button>
82+
83+
<AnimatePresence>
84+
{isPanelOpen && (
85+
<SlideOverPanel position="right">
86+
{(options: {isOpening: boolean}) => {
87+
return options.isOpening ? (
88+
<SkeletonPanelContents onClick={closePanel} />
89+
) : (
90+
<PanelContents onClick={closePanel} />
91+
);
92+
}}
93+
</SlideOverPanel>
94+
)}
95+
</AnimatePresence>
96+
```

‎static/app/components/core/slideOverPanel/slideOverPanel.tsx‎

Lines changed: 34 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {useCallback, useDeferredValue, useEffect} from 'react';
1+
import {useEffect, useState, useTransition} from 'react';
22
import isPropValid from '@emotion/is-prop-valid';
33
import {css, useTheme} from '@emotion/react';
44
import styled from '@emotion/styled';
@@ -30,17 +30,9 @@ type ChildRenderFunction = (renderPropProps: ChildRenderProps) => React.ReactNod
3030

3131
type SlideOverPanelProps = {
3232
children: React.ReactNode | ChildRenderFunction;
33-
/**
34-
* Whether the panel is visible. In most cases it's better to use this prop rather than render the panel conditionally, since it'll defer rendering the contents of the panel to a lower priority React lane.
35-
*/
36-
open: boolean;
3733
ariaLabel?: string;
3834
className?: string;
3935
'data-test-id'?: string;
40-
/**
41-
* Callback that fires every time the panel opens.
42-
*/
43-
onOpen?: () => void;
4436
panelWidth?: string;
4537
position?: 'right' | 'bottom' | 'left';
4638
ref?: React.Ref<HTMLDivElement>;
@@ -53,58 +45,51 @@ type SlideOverPanelProps = {
5345
export function SlideOverPanel({
5446
'data-test-id': testId,
5547
ariaLabel,
56-
open: isOpen,
5748
children,
5849
className,
59-
onOpen,
6050
position,
6151
transitionProps = {},
6252
panelWidth,
6353
ref,
6454
}: SlideOverPanelProps) {
6555
const theme = useTheme();
6656

67-
// Create a deferred version of `isOpen`. Here's how the rendering flow works
68-
// when the visibility changes to `true`:
57+
const [isTransitioning, startTransition] = useTransition();
58+
59+
// Defer rendering the children on initial mount. Here's how the flow works:
6960
//
70-
// 1. Parent component sets `isOpen` to `true`. This triggers a render.
71-
// 2. The render runs with `isOpen` `true` and `isContentVisible` `false`. We
72-
// pass `isOpening: true` to the `children` render prop. The render prop
73-
// contents should render a fast skeleton.
74-
// 3. The next render runs, with `isOpen` `true` and `isContentVisible`
75-
// `true`. This render is scheduled in the "transition" React lane. When this
76-
// is done, we pass `isOpening: false` to the children, and the render prop
77-
// should render the full UI. React periodically polls the main thread and
78-
// interrupt rendering of the full UI it if a high-priority render comes in.
79-
// Other state transitions are simpler, because we _only_ show the skeleton
80-
// during that "opening" transition.
81-
const isContentVisible = useDeferredValue(isOpen);
61+
// 1. On mount (the first render), `isContentVisible` is set to `false` and
62+
// `isTransitioning` set to `false`. We render the children and pass
63+
// `isOpening: true`. The children may choose to show a fast-to-render
64+
// skeleton UI, or nothing.
65+
// 2. Immediately after mount, `useEffect` runs and schedules a transition
66+
// that will update `isContentVisible` state to `true`.
67+
// 3. The "transition" starts. This component renders with `isContentVisible`
68+
// set to `false` but `isTransitioning` set to `true`, since the transition is
69+
// running.
70+
// 4. The transition makes the state update. This component re-renders with
71+
// `isTransitioning` set to `false` and `isContentVisible` set to `true`. We
72+
// render the children with `isOpening: false`. The children render their full
73+
// contents in a lower priority lane. When the render is complete, the content
74+
// is displayed.
75+
// Subsequent updates of the `children` are not deferred.
76+
const [isContentVisible, setIsContentVisible] = useState<boolean>(false);
8277

8378
useEffect(() => {
84-
if (isOpen && onOpen) {
85-
onOpen();
86-
}
87-
}, [isOpen, onOpen]);
88-
89-
// Create a fallback render function, in case the parent component passes
90-
// `React.ReactNode` as `children`. This way, even if they don't pass a render
91-
// prop they still get the benefit of fast panel opening.
92-
const fallbackRenderFunction = useCallback<ChildRenderFunction>(
93-
({isOpening}: ChildRenderProps) => {
94-
return isOpening ? null : (children as React.ReactNode); // This function should only ever run for `React.ReactNode`, its whole purpose is to create a render function for `React.ReactNode`
95-
},
96-
[children]
97-
);
79+
startTransition(() => {
80+
setIsContentVisible(true);
81+
});
82+
}, []);
9883

9984
const renderFunctionProps: ChildRenderProps = {
100-
isOpening: isOpen !== isContentVisible && !isContentVisible,
85+
isOpening: isTransitioning || !isContentVisible,
10186
};
10287

10388
const openStyle = position ? OPEN_STYLES[position] : OPEN_STYLES.right;
10489

10590
const collapsedStyle = position ? COLLAPSED_STYLES[position] : COLLAPSED_STYLES.right;
10691

107-
return isOpen ? (
92+
return (
10893
<_SlideOverPanel
10994
ref={ref}
11095
initial={collapsedStyle}
@@ -116,27 +101,28 @@ export function SlideOverPanel({
116101
...transitionProps,
117102
}}
118103
role="complementary"
119-
aria-hidden={!isOpen}
104+
aria-hidden={false}
120105
aria-label={ariaLabel ?? 'slide out drawer'}
121106
className={className}
122107
data-test-id={testId}
123108
panelWidth={panelWidth}
124109
>
125110
{/* Render the child content. If it's a render prop, pass the `isOpening`
126111
prop. We expect the render prop to render a skeleton UI if `isOpening` is
127-
true. Otherwise, use the fallback render prop, which shows a blank panel
128-
while opening. */}
112+
true. If `children` is not a render prop, render nothing while
113+
transitioning. */}
129114
{typeof children === 'function'
130115
? children(renderFunctionProps)
131-
: fallbackRenderFunction(renderFunctionProps)}
116+
: renderFunctionProps.isOpening
117+
? null
118+
: children}
132119
</_SlideOverPanel>
133-
) : null;
120+
);
134121
}
135122

136123
const _SlideOverPanel = styled(motion.div, {
137124
shouldForwardProp: prop =>
138-
['initial', 'animate', 'exit', 'transition'].includes(prop) ||
139-
(prop !== 'isOpen' && isPropValid(prop)),
125+
['initial', 'animate', 'exit', 'transition'].includes(prop) || isPropValid(prop),
140126
})<{
141127
panelWidth?: string;
142128
position?: 'right' | 'bottom' | 'left';

‎static/app/components/globalDrawer/components.tsx‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@ function DrawerPanel({
7575
<DrawerSlidePanel
7676
ariaLabel={ariaLabel}
7777
position="right"
78-
open
7978
ref={mergeRefs(panelRef, ref)}
8079
transitionProps={transitionProps}
8180
panelWidth="var(--drawer-width)" // Initial width only

‎static/app/stories/playground/slideOverPanel.tsx‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {Fragment, useCallback, useState, type ComponentProps} from 'react';
2+
import {AnimatePresence} from 'framer-motion';
23

34
import {Alert} from '@sentry/scraps/alert';
45
import {Button} from '@sentry/scraps/button';
@@ -14,11 +15,13 @@ export function SlideOverPanelPlayground() {
1415
<Fragment>
1516
<Button onClick={() => setIsPanelOpen(true)}>Open Panel</Button>
1617

17-
<SlideOverPanel open={isPanelOpen} position="right">
18-
<Container border="primary" height="100%" padding="md">
19-
<Button onClick={() => setIsPanelOpen(false)}>Close Panel</Button>
20-
</Container>
21-
</SlideOverPanel>
18+
{isPanelOpen && (
19+
<SlideOverPanel position="right">
20+
<Container border="primary" height="100%" padding="md">
21+
<Button onClick={() => setIsPanelOpen(false)}>Close Panel</Button>
22+
</Container>
23+
</SlideOverPanel>
24+
)}
2225
</Fragment>
2326
);
2427
}
@@ -34,15 +37,19 @@ export function SlideOverPanelSkeletonPlayground() {
3437
<Fragment>
3538
<Button onClick={() => setIsPanelOpen(true)}>Open Panel</Button>
3639

37-
<SlideOverPanel open={isPanelOpen} position="right">
38-
{(options: {isOpening: boolean}) => {
39-
return options.isOpening ? (
40-
<SkeletonPanelContents onClick={closePanel} />
41-
) : (
42-
<PanelContents onClick={closePanel} />
43-
);
44-
}}
45-
</SlideOverPanel>
40+
<AnimatePresence>
41+
{isPanelOpen && (
42+
<SlideOverPanel position="right">
43+
{(options: {isOpening: boolean}) => {
44+
return options.isOpening ? (
45+
<SkeletonPanelContents onClick={closePanel} />
46+
) : (
47+
<PanelContents onClick={closePanel} />
48+
);
49+
}}
50+
</SlideOverPanel>
51+
)}
52+
</AnimatePresence>
4653
</Fragment>
4754
);
4855
}

‎static/app/views/dashboards/widgetBuilder/components/widgetBuilderSlideout.tsx‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,6 @@ function WidgetBuilderSlideout({
209209

210210
return (
211211
<SlideOverPanel
212-
open
213212
position="left"
214213
data-test-id="widget-slideout"
215214
transitionProps={animationTransitionSettings}

0 commit comments

Comments
 (0)