Skip to content

Commit

Permalink
feat(insights): Use GlobalDrawer instead of details panel II (#83301)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
gggritso authored Jan 13, 2025
1 parent 49ba6fe commit 48826bf
Show file tree
Hide file tree
Showing 30 changed files with 620 additions and 582 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {ModuleBodyUpsellHook} from 'sentry/views/insights/common/components/modu
import {ToolRibbon} from 'sentry/views/insights/common/components/ribbon';
import {useSpanMetrics} from 'sentry/views/insights/common/queries/useDiscover';
import {useModuleURL} from 'sentry/views/insights/common/utils/useModuleURL';
import {useSamplesDrawer} from 'sentry/views/insights/common/utils/useSamplesDrawer';
import SubregionSelector from 'sentry/views/insights/common/views/spans/selectors/subregionSelector';
import {SampleList} from 'sentry/views/insights/common/views/spanSummaryPage/sampleList';
import {FrontendHeader} from 'sentry/views/insights/pages/frontend/frontendPageHeader';
Expand All @@ -43,6 +44,20 @@ function ResourceSummary() {
const {groupId} = useParams();
const filters = useResourceModuleFilters();
const selectedSpanOp = filters[SPAN_OP];

useSamplesDrawer({
Component: (
<SampleList
groupId={groupId!}
moduleName={ModuleName.RESOURCE}
transactionRoute={webVitalsModuleURL}
referrer={TraceViewSources.ASSETS_MODULE}
/>
),
moduleName: ModuleName.RESOURCE,
requiredParams: ['transaction'],
});

const {data, isPending} = useSpanMetrics(
{
search: MutableSearch.fromQueryObject({
Expand Down Expand Up @@ -135,15 +150,6 @@ function ResourceSummary() {
<ModuleLayout.Full>
<ResourceSummaryTable />
</ModuleLayout.Full>

<ModuleLayout.Full>
<SampleList
groupId={groupId!}
moduleName={ModuleName.RESOURCE}
transactionRoute={webVitalsModuleURL}
referrer={TraceViewSources.ASSETS_MODULE}
/>
</ModuleLayout.Full>
</ModuleLayout.Layout>
</Layout.Main>
</Layout.Body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {useMemo} from 'react';
import styled from '@emotion/styled';

import type {LineChartSeries} from 'sentry/components/charts/lineChart';
import {DrawerHeader} from 'sentry/components/globalDrawer/components';
import type {
GridColumnHeader,
GridColumnOrder,
Expand Down Expand Up @@ -38,7 +39,7 @@ import type {
} from 'sentry/views/insights/browser/webVitals/types';
import decodeBrowserTypes from 'sentry/views/insights/browser/webVitals/utils/queryParameterDecoders/browserType';
import useProfileExists from 'sentry/views/insights/browser/webVitals/utils/useProfileExists';
import DetailPanel from 'sentry/views/insights/common/components/detailPanel';
import {SampleDrawerBody} from 'sentry/views/insights/common/components/sampleDrawerBody';
import {SpanIndexedField, type SubregionCode} from 'sentry/views/insights/types';
import {TraceViewSources} from 'sentry/views/performance/newTraceDetails/traceHeader/breadcrumbs';
import {generateReplayLink} from 'sentry/views/performance/transactionSummary/utils';
Expand Down Expand Up @@ -77,9 +78,7 @@ const inpSort: GridColumnSortBy<keyof InteractionSpanSampleRowWithScore> = {

export function PageOverviewWebVitalsDetailPanel({
webVital,
onClose,
}: {
onClose: () => void;
webVital: WebVitals | null;
}) {
const location = useLocation();
Expand Down Expand Up @@ -367,7 +366,9 @@ export function PageOverviewWebVitalsDetailPanel({

return (
<PageAlertProvider>
<DetailPanel detailKey={webVital ?? undefined} onClose={onClose}>
<DrawerHeader />

<SampleDrawerBody>
{webVital && (
<WebVitalDetailHeader
value={
Expand Down Expand Up @@ -410,7 +411,7 @@ export function PageOverviewWebVitalsDetailPanel({
)}
</TableContainer>
<PageAlert />
</DetailPanel>
</SampleDrawerBody>
</PageAlertProvider>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,6 @@ const Value = styled('h2')`

const WebVitalName = styled('h4')`
margin-bottom: ${space(1)};
margin-top: 40px;
max-width: 400px;
${p => p.theme.overflowEllipsis}
`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe('WebVitalsDetailPanel', function () {
});

it('renders correctly with empty results', async () => {
render(<WebVitalsDetailPanel onClose={() => undefined} webVital="lcp" />, {
render(<WebVitalsDetailPanel webVital="lcp" />, {
organization,
});
await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator'));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {useEffect, useMemo} from 'react';
import styled from '@emotion/styled';

import type {LineChartSeries} from 'sentry/components/charts/lineChart';
import {DrawerHeader} from 'sentry/components/globalDrawer/components';
import type {
GridColumnHeader,
GridColumnOrder,
Expand Down Expand Up @@ -34,7 +35,7 @@ import type {
WebVitals,
} from 'sentry/views/insights/browser/webVitals/types';
import decodeBrowserTypes from 'sentry/views/insights/browser/webVitals/utils/queryParameterDecoders/browserType';
import DetailPanel from 'sentry/views/insights/common/components/detailPanel';
import {SampleDrawerBody} from 'sentry/views/insights/common/components/sampleDrawerBody';
import {SpanIndexedField, type SubregionCode} from 'sentry/views/insights/types';

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

const MAX_ROWS = 10;

export function WebVitalsDetailPanel({
webVital,
onClose,
}: {
onClose: () => void;
webVital: WebVitals | null;
}) {
export function WebVitalsDetailPanel({webVital}: {webVital: WebVitals | null}) {
const location = useLocation();
const organization = useOrganization();
const browserTypes = decodeBrowserTypes(location.query[SpanIndexedField.BROWSER_NAME]);
Expand Down Expand Up @@ -133,8 +128,6 @@ export function WebVitalsDetailPanel({
seriesName: webVital ?? '',
};

const detailKey = webVital;

useEffect(() => {
if (webVital !== null) {
trackAnalytics('insight.vital.vital_sidebar_opened', {
Expand Down Expand Up @@ -243,7 +236,9 @@ export function WebVitalsDetailPanel({

return (
<PageAlertProvider>
<DetailPanel detailKey={detailKey ?? undefined} onClose={onClose}>
<DrawerHeader />

<SampleDrawerBody>
{webVital && (
<WebVitalDescription
value={
Expand Down Expand Up @@ -274,7 +269,7 @@ export function WebVitalsDetailPanel({
/>
</TableContainer>
<PageAlert />
</DetailPanel>
</SampleDrawerBody>
</PageAlertProvider>
);
}
Expand Down
32 changes: 21 additions & 11 deletions static/app/views/insights/browser/webVitals/views/pageOverview.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {Fragment, useMemo, useState} from 'react';
import React, {Fragment, useEffect, useMemo, useState} from 'react';
import styled from '@emotion/styled';
import omit from 'lodash/omit';

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

const {openVitalsDrawer} = useWebVitalsDrawer({
Component: <PageOverviewWebVitalsDetailPanel webVital={state.webVital} />,
webVital: state.webVital,
onClose: () => {
router.replace({
pathname: router.location.pathname,
query: omit(router.location.query, 'webVital'),
});

setState({...state, webVital: null});
},
});

useEffect(() => {
if (state.webVital) {
openVitalsDrawer();
}
});

if (transaction === undefined) {
// redirect user to webvitals landing page
window.location.href = moduleURL;
Expand Down Expand Up @@ -239,16 +259,6 @@ export function PageOverview() {
</Layout.Body>
)}
</ModuleBodyUpsellHook>
<PageOverviewWebVitalsDetailPanel
webVital={state.webVital}
onClose={() => {
router.replace({
pathname: router.location.pathname,
query: omit(router.location.query, 'webVital'),
});
setState({...state, webVital: null});
}}
/>
</Tabs>
</React.Fragment>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React, {Fragment, useState} from 'react';
import React, {Fragment, useEffect, useState} from 'react';
import styled from '@emotion/styled';
import omit from 'lodash/omit';

import Alert from 'sentry/components/alert';
import {Button} from 'sentry/components/button';
Expand All @@ -11,7 +10,6 @@ import {t, tct} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import {decodeList} from 'sentry/utils/queryString';
import {useLocation} from 'sentry/utils/useLocation';
import useRouter from 'sentry/utils/useRouter';
import BrowserTypeSelector from 'sentry/views/insights/browser/webVitals/components/browserTypeSelector';
import {PerformanceScoreChart} from 'sentry/views/insights/browser/webVitals/components/charts/performanceScoreChart';
import {PagePerformanceTable} from 'sentry/views/insights/browser/webVitals/components/tables/pagePerformanceTable';
Expand All @@ -26,6 +24,7 @@ import {ModulePageFilterBar} from 'sentry/views/insights/common/components/modul
import {ModulePageProviders} from 'sentry/views/insights/common/components/modulePageProviders';
import {ModulesOnboarding} from 'sentry/views/insights/common/components/modulesOnboarding';
import {ModuleBodyUpsellHook} from 'sentry/views/insights/common/components/moduleUpsellHookWrapper';
import {useWebVitalsDrawer} from 'sentry/views/insights/common/utils/useWebVitalsDrawer';
import {FrontendHeader} from 'sentry/views/insights/pages/frontend/frontendPageHeader';
import {
ModuleName,
Expand All @@ -38,8 +37,6 @@ const WEB_VITALS_COUNT = 5;
export function WebVitalsLandingPage() {
const location = useLocation();

const router = useRouter();

const [state, setState] = useState<{webVital: WebVitals | null}>({
webVital: (location.query.webVital as WebVitals) ?? null,
});
Expand All @@ -61,6 +58,20 @@ export function WebVitalsLandingPage() {
? undefined
: getWebVitalScoresFromTableDataRow(projectScores?.data?.[0]);

const {openVitalsDrawer} = useWebVitalsDrawer({
Component: <WebVitalsDetailPanel webVital={state.webVital} />,
webVital: state.webVital,
onClose: () => {
setState({webVital: null});
},
});

useEffect(() => {
if (state.webVital) {
openVitalsDrawer();
}
});

return (
<React.Fragment>
<FrontendHeader module={ModuleName.VITAL} />
Expand Down Expand Up @@ -130,16 +141,6 @@ export function WebVitalsLandingPage() {
</Layout.Main>
</Layout.Body>
</ModuleBodyUpsellHook>
<WebVitalsDetailPanel
webVital={state.webVital}
onClose={() => {
router.replace({
pathname: router.location.pathname,
query: omit(router.location.query, 'webVital'),
});
setState({...state, webVital: null});
}}
/>
</React.Fragment>
);
}
Expand Down
Loading

0 comments on commit 48826bf

Please sign in to comment.