Skip to content

Commit

Permalink
fix(perf): Do not render contents of closed panels (#70286)
Browse files Browse the repository at this point in the history
Right now, when a `SlideOverPanel` is collapsed, its contents still
exist in the DOM. This has a whole bunch of downsides:

1. React has to render stuff inside, which is pointless since it's
invisible
2. When writing mocks, we have to assert on elements that might exist in
the panel even if its closed

This is very annoying! This PR changes it up to use `AnimatePresence`
which lets us use a `!collapsed &` condition and _not_ render the
panel's contents in the DOM if it's collapsed.

Also clarified some variables names while I'm at it, and removed some
unused CSS.
  • Loading branch information
gggritso authored May 6, 2024
1 parent 3609aff commit b61c843
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 34 deletions.
22 changes: 22 additions & 0 deletions static/app/views/starfish/components/detailPanel.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import {render, screen} from 'sentry-test/reactTestingLibrary';

import DetailPanel from 'sentry/views/starfish/components/detailPanel';

describe('DetailPanel', function () {
it('renders toolbar and inner content', function () {
render(<DetailPanel detailKey={'true'}>Content</DetailPanel>);

expect(screen.getByRole('button', {name: 'Dock to the bottom'})).toBeInTheDocument();
expect(screen.getByRole('button', {name: 'Dock to the right'})).toBeInTheDocument();
expect(screen.getByRole('button', {name: 'Close Details'})).toBeInTheDocument();

expect(screen.getByText('Content')).toBeInTheDocument();
});

it('does not render content when closed', function () {
render(<DetailPanel detailKey={undefined}>Content</DetailPanel>);

expect(screen.queryByRole('button', {name: 'Close Details'})).not.toBeInTheDocument();
expect(screen.queryByText('Content')).not.toBeInTheDocument();
});
});
65 changes: 31 additions & 34 deletions static/app/views/starfish/components/slideOverPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@ import {forwardRef, useEffect} from 'react';
import isPropValid from '@emotion/is-prop-valid';
import {css} from '@emotion/react';
import styled from '@emotion/styled';
import {motion} from 'framer-motion';
import {AnimatePresence, motion} from 'framer-motion';

import {space} from 'sentry/styles/space';

const PANEL_WIDTH = '50vw';
const PANEL_HEIGHT = '50vh';

const INITIAL_STYLES = {
bottom: {opacity: 0, x: 0, y: 0},
right: {opacity: 0, x: PANEL_WIDTH, y: 0},
const OPEN_STYLES = {
bottom: {opacity: 1, x: 0, y: 0},
right: {opacity: 1, x: 0, y: 0},
};

const FINAL_STYLES = {
const COLLAPSED_STYLES = {
bottom: {opacity: 0, x: 0, y: PANEL_HEIGHT},
right: {opacity: 0, x: PANEL_WIDTH},
right: {opacity: 0, x: PANEL_WIDTH, y: 0},
};

type SlideOverPanelProps = {
Expand All @@ -38,33 +38,40 @@ function SlideOverPanel(
onOpen();
}
}, [collapsed, onOpen]);
const initial = slidePosition ? INITIAL_STYLES[slidePosition] : INITIAL_STYLES.right;
const final = slidePosition ? FINAL_STYLES[slidePosition] : FINAL_STYLES.right;

const openStyle = slidePosition ? OPEN_STYLES[slidePosition] : OPEN_STYLES.right;

const collapsedStyle = slidePosition
? COLLAPSED_STYLES[slidePosition]
: COLLAPSED_STYLES.right;

return (
<_SlideOverPanel
ref={ref}
collapsed={collapsed}
initial={initial}
animate={!collapsed ? {opacity: 1, x: 0, y: 0} : final}
slidePosition={slidePosition}
transition={{
type: 'spring',
stiffness: 500,
damping: 50,
}}
>
{children}
</_SlideOverPanel>
<AnimatePresence>
{!collapsed && (
<_SlideOverPanel
ref={ref}
initial={collapsedStyle}
animate={openStyle}
exit={collapsedStyle}
slidePosition={slidePosition}
transition={{
type: 'spring',
stiffness: 500,
damping: 50,
}}
>
{children}
</_SlideOverPanel>
)}
</AnimatePresence>
);
}

const _SlideOverPanel = styled(motion.div, {
shouldForwardProp: prop =>
['animate', 'transition', 'initial'].includes(prop) ||
['initial', 'animate', 'exit', 'transition'].includes(prop) ||
(prop !== 'collapsed' && isPropValid(prop)),
})<{
collapsed: boolean;
slidePosition?: 'right' | 'bottom';
}>`
position: fixed;
Expand Down Expand Up @@ -109,14 +116,4 @@ const _SlideOverPanel = styled(motion.div, {
left: auto;
`}
}
${p =>
p.collapsed
? css`
overflow: hidden;
`
: css`
overflow-x: hidden;
overflow-y: auto;
`}
`;

0 comments on commit b61c843

Please sign in to comment.