Skip to content

Commit 48826bf

Browse files
authored
feat(insights): Use GlobalDrawer instead of details panel II (#83301)
Attempt #2. The [previous PR](#82534) had to be reverted because I didn't account for URL behaviour. This PR is very similar, but now opening and closing the span samples drawer is done in a special hook that watches for the required URL parameters. --- Closes #81273 Replaces all Insights side panels with new `GlobalDrawer` implementations that match the drawers in Issues. The visual changes are minimal 1. Slightly different animations 2. A "Close" button on the top left of the drawer 3. Scroll bars on the left of the drawer 4. Tighter padding 5. Panel doesn't re-animate as often on clicking another transaction There are small feature changes! - no more docking bottom or right. Docking is to the right only - panels now have closing animations **Before:** <img width="829" alt="Screenshot 2024-12-23 at 11 42 33 AM" src="https://github.com/user-attachments/assets/78e3631b-7694-42eb-b3f0-149f92307748" /> <img width="817" alt="Screenshot 2024-12-23 at 11 42 44 AM" src="https://github.com/user-attachments/assets/f24b2a0e-3adb-414d-b965-eeb5715f7a9e" /> <img width="838" alt="Screenshot 2024-12-23 at 11 43 07 AM" src="https://github.com/user-attachments/assets/9753b6da-a6fb-464b-bc3e-25f5126fa4e5" /> **After:** <img width="767" alt="Screenshot 2024-12-23 at 11 43 19 AM" src="https://github.com/user-attachments/assets/b500af97-7493-4f2f-9330-6190e31eaac1" /> <img width="774" alt="Screenshot 2024-12-23 at 11 43 55 AM" src="https://github.com/user-attachments/assets/332569ad-b715-4365-b000-188b64747944" /> <img width="563" alt="Screenshot 2024-12-23 at 11 44 44 AM" src="https://github.com/user-attachments/assets/338ec721-471e-4760-b028-6df850df0e8e" /> <img width="483" alt="Screenshot 2024-12-23 at 12 04 50 PM" src="https://github.com/user-attachments/assets/db20d3a8-9958-4993-8ea0-6781e1467a80" /> ## Code Changes `GlobalDrawer` is invoked with a hook, rather than rendered inline. So, I had to make some changes. **Before:** - rendering panels mostly unconditionally like `<CacheSamplesPanel />` - each sample panel checks the URL to see if it's supposed to be open - if open, renders itself, otherwise stays invisible - on close, updates the URL, which re-renders and hides This is problematic for a bunch of reasons: 1. When components can decide to not render themselves, the code flow is confusing. Parents should generally decide when to render children 2. When a component is rendered but hidden, it runs its side effects. This means that in our case, network requests were sometimes going out, even though the panel isn't visible **After:** - the panel is rendered using a `useEffect`, the effect detects a necessary URL parameter or state, and open the panel if necessary. All conditions that the panel used to check for itself are passed as parameters to the hooks - the panel is rendered straight, it doesn't have any conditionals to check if it's supposed to be open - on close, the panel hides itself, and tells the parent to update the URL or state as necessary This is tidier, avoids component side-effects, and preserves closing animations! Plus, this means I can re-use a header component, make the analytics tracking consistent, etc. --- This only leaves a small handful of users of `DetailPanel`, mostly in the trace view.
1 parent 49ba6fe commit 48826bf

30 files changed

+620
-582
lines changed

static/app/views/insights/browser/resources/views/resourceSummaryPage.tsx

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {ModuleBodyUpsellHook} from 'sentry/views/insights/common/components/modu
2222
import {ToolRibbon} from 'sentry/views/insights/common/components/ribbon';
2323
import {useSpanMetrics} from 'sentry/views/insights/common/queries/useDiscover';
2424
import {useModuleURL} from 'sentry/views/insights/common/utils/useModuleURL';
25+
import {useSamplesDrawer} from 'sentry/views/insights/common/utils/useSamplesDrawer';
2526
import SubregionSelector from 'sentry/views/insights/common/views/spans/selectors/subregionSelector';
2627
import {SampleList} from 'sentry/views/insights/common/views/spanSummaryPage/sampleList';
2728
import {FrontendHeader} from 'sentry/views/insights/pages/frontend/frontendPageHeader';
@@ -43,6 +44,20 @@ function ResourceSummary() {
4344
const {groupId} = useParams();
4445
const filters = useResourceModuleFilters();
4546
const selectedSpanOp = filters[SPAN_OP];
47+
48+
useSamplesDrawer({
49+
Component: (
50+
<SampleList
51+
groupId={groupId!}
52+
moduleName={ModuleName.RESOURCE}
53+
transactionRoute={webVitalsModuleURL}
54+
referrer={TraceViewSources.ASSETS_MODULE}
55+
/>
56+
),
57+
moduleName: ModuleName.RESOURCE,
58+
requiredParams: ['transaction'],
59+
});
60+
4661
const {data, isPending} = useSpanMetrics(
4762
{
4863
search: MutableSearch.fromQueryObject({
@@ -135,15 +150,6 @@ function ResourceSummary() {
135150
<ModuleLayout.Full>
136151
<ResourceSummaryTable />
137152
</ModuleLayout.Full>
138-
139-
<ModuleLayout.Full>
140-
<SampleList
141-
groupId={groupId!}
142-
moduleName={ModuleName.RESOURCE}
143-
transactionRoute={webVitalsModuleURL}
144-
referrer={TraceViewSources.ASSETS_MODULE}
145-
/>
146-
</ModuleLayout.Full>
147153
</ModuleLayout.Layout>
148154
</Layout.Main>
149155
</Layout.Body>

static/app/views/insights/browser/webVitals/components/pageOverviewWebVitalsDetailPanel.tsx

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import {useMemo} from 'react';
22
import styled from '@emotion/styled';
33

44
import type {LineChartSeries} from 'sentry/components/charts/lineChart';
5+
import {DrawerHeader} from 'sentry/components/globalDrawer/components';
56
import type {
67
GridColumnHeader,
78
GridColumnOrder,
@@ -38,7 +39,7 @@ import type {
3839
} from 'sentry/views/insights/browser/webVitals/types';
3940
import decodeBrowserTypes from 'sentry/views/insights/browser/webVitals/utils/queryParameterDecoders/browserType';
4041
import useProfileExists from 'sentry/views/insights/browser/webVitals/utils/useProfileExists';
41-
import DetailPanel from 'sentry/views/insights/common/components/detailPanel';
42+
import {SampleDrawerBody} from 'sentry/views/insights/common/components/sampleDrawerBody';
4243
import {SpanIndexedField, type SubregionCode} from 'sentry/views/insights/types';
4344
import {TraceViewSources} from 'sentry/views/performance/newTraceDetails/traceHeader/breadcrumbs';
4445
import {generateReplayLink} from 'sentry/views/performance/transactionSummary/utils';
@@ -77,9 +78,7 @@ const inpSort: GridColumnSortBy<keyof InteractionSpanSampleRowWithScore> = {
7778

7879
export function PageOverviewWebVitalsDetailPanel({
7980
webVital,
80-
onClose,
8181
}: {
82-
onClose: () => void;
8382
webVital: WebVitals | null;
8483
}) {
8584
const location = useLocation();
@@ -367,7 +366,9 @@ export function PageOverviewWebVitalsDetailPanel({
367366

368367
return (
369368
<PageAlertProvider>
370-
<DetailPanel detailKey={webVital ?? undefined} onClose={onClose}>
369+
<DrawerHeader />
370+
371+
<SampleDrawerBody>
371372
{webVital && (
372373
<WebVitalDetailHeader
373374
value={
@@ -410,7 +411,7 @@ export function PageOverviewWebVitalsDetailPanel({
410411
)}
411412
</TableContainer>
412413
<PageAlert />
413-
</DetailPanel>
414+
</SampleDrawerBody>
414415
</PageAlertProvider>
415416
);
416417
}

static/app/views/insights/browser/webVitals/components/webVitalDescription.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,6 @@ const Value = styled('h2')`
273273

274274
const WebVitalName = styled('h4')`
275275
margin-bottom: ${space(1)};
276-
margin-top: 40px;
277276
max-width: 400px;
278277
${p => p.theme.overflowEllipsis}
279278
`;

static/app/views/insights/browser/webVitals/components/webVitalsDetailPanel.spec.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ describe('WebVitalsDetailPanel', function () {
5858
});
5959

6060
it('renders correctly with empty results', async () => {
61-
render(<WebVitalsDetailPanel onClose={() => undefined} webVital="lcp" />, {
61+
render(<WebVitalsDetailPanel webVital="lcp" />, {
6262
organization,
6363
});
6464
await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator'));

static/app/views/insights/browser/webVitals/components/webVitalsDetailPanel.tsx

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import {useEffect, useMemo} from 'react';
22
import styled from '@emotion/styled';
33

44
import type {LineChartSeries} from 'sentry/components/charts/lineChart';
5+
import {DrawerHeader} from 'sentry/components/globalDrawer/components';
56
import type {
67
GridColumnHeader,
78
GridColumnOrder,
@@ -34,7 +35,7 @@ import type {
3435
WebVitals,
3536
} from 'sentry/views/insights/browser/webVitals/types';
3637
import decodeBrowserTypes from 'sentry/views/insights/browser/webVitals/utils/queryParameterDecoders/browserType';
37-
import DetailPanel from 'sentry/views/insights/common/components/detailPanel';
38+
import {SampleDrawerBody} from 'sentry/views/insights/common/components/sampleDrawerBody';
3839
import {SpanIndexedField, type SubregionCode} from 'sentry/views/insights/types';
3940

4041
type Column = GridColumnHeader;
@@ -51,13 +52,7 @@ const sort: GridColumnSortBy<keyof Row> = {key: 'count()', order: 'desc'};
5152

5253
const MAX_ROWS = 10;
5354

54-
export function WebVitalsDetailPanel({
55-
webVital,
56-
onClose,
57-
}: {
58-
onClose: () => void;
59-
webVital: WebVitals | null;
60-
}) {
55+
export function WebVitalsDetailPanel({webVital}: {webVital: WebVitals | null}) {
6156
const location = useLocation();
6257
const organization = useOrganization();
6358
const browserTypes = decodeBrowserTypes(location.query[SpanIndexedField.BROWSER_NAME]);
@@ -133,8 +128,6 @@ export function WebVitalsDetailPanel({
133128
seriesName: webVital ?? '',
134129
};
135130

136-
const detailKey = webVital;
137-
138131
useEffect(() => {
139132
if (webVital !== null) {
140133
trackAnalytics('insight.vital.vital_sidebar_opened', {
@@ -243,7 +236,9 @@ export function WebVitalsDetailPanel({
243236

244237
return (
245238
<PageAlertProvider>
246-
<DetailPanel detailKey={detailKey ?? undefined} onClose={onClose}>
239+
<DrawerHeader />
240+
241+
<SampleDrawerBody>
247242
{webVital && (
248243
<WebVitalDescription
249244
value={
@@ -274,7 +269,7 @@ export function WebVitalsDetailPanel({
274269
/>
275270
</TableContainer>
276271
<PageAlert />
277-
</DetailPanel>
272+
</SampleDrawerBody>
278273
</PageAlertProvider>
279274
);
280275
}

static/app/views/insights/browser/webVitals/views/pageOverview.tsx

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import React, {Fragment, useMemo, useState} from 'react';
1+
import React, {Fragment, useEffect, useMemo, useState} from 'react';
22
import styled from '@emotion/styled';
33
import omit from 'lodash/omit';
44

@@ -32,6 +32,7 @@ import {ModulePageFilterBar} from 'sentry/views/insights/common/components/modul
3232
import {ModulePageProviders} from 'sentry/views/insights/common/components/modulePageProviders';
3333
import {ModuleBodyUpsellHook} from 'sentry/views/insights/common/components/moduleUpsellHookWrapper';
3434
import {useModuleURL} from 'sentry/views/insights/common/utils/useModuleURL';
35+
import {useWebVitalsDrawer} from 'sentry/views/insights/common/utils/useWebVitalsDrawer';
3536
import {FrontendHeader} from 'sentry/views/insights/pages/frontend/frontendPageHeader';
3637
import {useDomainViewFilters} from 'sentry/views/insights/pages/useFilters';
3738
import {
@@ -106,6 +107,25 @@ export function PageOverview() {
106107
const {data: projectScores, isPending: isProjectScoresLoading} =
107108
useProjectWebVitalsScoresQuery({transaction, browserTypes, subregions});
108109

110+
const {openVitalsDrawer} = useWebVitalsDrawer({
111+
Component: <PageOverviewWebVitalsDetailPanel webVital={state.webVital} />,
112+
webVital: state.webVital,
113+
onClose: () => {
114+
router.replace({
115+
pathname: router.location.pathname,
116+
query: omit(router.location.query, 'webVital'),
117+
});
118+
119+
setState({...state, webVital: null});
120+
},
121+
});
122+
123+
useEffect(() => {
124+
if (state.webVital) {
125+
openVitalsDrawer();
126+
}
127+
});
128+
109129
if (transaction === undefined) {
110130
// redirect user to webvitals landing page
111131
window.location.href = moduleURL;
@@ -239,16 +259,6 @@ export function PageOverview() {
239259
</Layout.Body>
240260
)}
241261
</ModuleBodyUpsellHook>
242-
<PageOverviewWebVitalsDetailPanel
243-
webVital={state.webVital}
244-
onClose={() => {
245-
router.replace({
246-
pathname: router.location.pathname,
247-
query: omit(router.location.query, 'webVital'),
248-
});
249-
setState({...state, webVital: null});
250-
}}
251-
/>
252262
</Tabs>
253263
</React.Fragment>
254264
);

static/app/views/insights/browser/webVitals/views/webVitalsLandingPage.tsx

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
import React, {Fragment, useState} from 'react';
1+
import React, {Fragment, useEffect, useState} from 'react';
22
import styled from '@emotion/styled';
3-
import omit from 'lodash/omit';
43

54
import Alert from 'sentry/components/alert';
65
import {Button} from 'sentry/components/button';
@@ -11,7 +10,6 @@ import {t, tct} from 'sentry/locale';
1110
import {space} from 'sentry/styles/space';
1211
import {decodeList} from 'sentry/utils/queryString';
1312
import {useLocation} from 'sentry/utils/useLocation';
14-
import useRouter from 'sentry/utils/useRouter';
1513
import BrowserTypeSelector from 'sentry/views/insights/browser/webVitals/components/browserTypeSelector';
1614
import {PerformanceScoreChart} from 'sentry/views/insights/browser/webVitals/components/charts/performanceScoreChart';
1715
import {PagePerformanceTable} from 'sentry/views/insights/browser/webVitals/components/tables/pagePerformanceTable';
@@ -26,6 +24,7 @@ import {ModulePageFilterBar} from 'sentry/views/insights/common/components/modul
2624
import {ModulePageProviders} from 'sentry/views/insights/common/components/modulePageProviders';
2725
import {ModulesOnboarding} from 'sentry/views/insights/common/components/modulesOnboarding';
2826
import {ModuleBodyUpsellHook} from 'sentry/views/insights/common/components/moduleUpsellHookWrapper';
27+
import {useWebVitalsDrawer} from 'sentry/views/insights/common/utils/useWebVitalsDrawer';
2928
import {FrontendHeader} from 'sentry/views/insights/pages/frontend/frontendPageHeader';
3029
import {
3130
ModuleName,
@@ -38,8 +37,6 @@ const WEB_VITALS_COUNT = 5;
3837
export function WebVitalsLandingPage() {
3938
const location = useLocation();
4039

41-
const router = useRouter();
42-
4340
const [state, setState] = useState<{webVital: WebVitals | null}>({
4441
webVital: (location.query.webVital as WebVitals) ?? null,
4542
});
@@ -61,6 +58,20 @@ export function WebVitalsLandingPage() {
6158
? undefined
6259
: getWebVitalScoresFromTableDataRow(projectScores?.data?.[0]);
6360

61+
const {openVitalsDrawer} = useWebVitalsDrawer({
62+
Component: <WebVitalsDetailPanel webVital={state.webVital} />,
63+
webVital: state.webVital,
64+
onClose: () => {
65+
setState({webVital: null});
66+
},
67+
});
68+
69+
useEffect(() => {
70+
if (state.webVital) {
71+
openVitalsDrawer();
72+
}
73+
});
74+
6475
return (
6576
<React.Fragment>
6677
<FrontendHeader module={ModuleName.VITAL} />
@@ -130,16 +141,6 @@ export function WebVitalsLandingPage() {
130141
</Layout.Main>
131142
</Layout.Body>
132143
</ModuleBodyUpsellHook>
133-
<WebVitalsDetailPanel
134-
webVital={state.webVital}
135-
onClose={() => {
136-
router.replace({
137-
pathname: router.location.pathname,
138-
query: omit(router.location.query, 'webVital'),
139-
});
140-
setState({...state, webVital: null});
141-
}}
142-
/>
143144
</React.Fragment>
144145
);
145146
}

0 commit comments

Comments
 (0)