Skip to content

Commit 028d49a

Browse files
authored
breadcrumbs: remove styled and simplify (#102980)
Simplify breadcrumb component and add amplitude logging. - Removed optional key prop - Use layout and text components for styling - Improve prop types so that null labels cannot be passed to components - Add amplitude event tracking so we can gauge how many folks interact with our breadcrumbs
1 parent 652a851 commit 028d49a

File tree

14 files changed

+132
-211
lines changed

14 files changed

+132
-211
lines changed

static/app/components/breadcrumbs.stories.tsx

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -15,34 +15,19 @@ export default Storybook.story('Breadcrumbs', story => {
1515
<Breadcrumbs
1616
crumbs={[
1717
{label: 'Organization', to: '/organizations/sentry/'},
18-
{label: 'Projects', to: '/organizations/sentry/projects/'},
19-
{label: 'Project Settings', to: '/settings/projects/javascript/'},
18+
{label: 'Projects'},
19+
{
20+
label: 'Project Settings',
21+
to: '/settings/projects/javascript/',
22+
preservePageFilters: true,
23+
},
2024
{label: 'General', to: null},
2125
]}
2226
/>
2327
</Storybook.SizingWindow>
2428
</Fragment>
2529
));
2630

27-
story('With Last Item Linked', () => (
28-
<Fragment>
29-
<p>
30-
Set <Storybook.JSXProperty name="linkLastItem" value /> to make the last
31-
breadcrumb clickable.
32-
</p>
33-
<Storybook.SizingWindow display="block">
34-
<Breadcrumbs
35-
linkLastItem
36-
crumbs={[
37-
{label: 'Organization', to: '/organizations/sentry/'},
38-
{label: 'Projects', to: '/organizations/sentry/projects/'},
39-
{label: 'All Projects', to: '/organizations/sentry/projects/all/'},
40-
]}
41-
/>
42-
</Storybook.SizingWindow>
43-
</Fragment>
44-
));
45-
4631
story('Page Filter Preservation', () => (
4732
<Fragment>
4833
<p>
@@ -87,7 +72,8 @@ export default Storybook.story('Breadcrumbs', story => {
8772
],
8873
[
8974
{
90-
label: 'longlonglonglonglonglonglonglonglonglonglonglonglonglonglong',
75+
label:
76+
'A Very Long Project Name Here That Will Be Truncated Because It Is Too Long',
9177
to: '/org/',
9278
},
9379
{label: 'Very Long Project Name Here', to: '/project/'},
Lines changed: 73 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,19 @@
11
import {Fragment} from 'react';
2-
import type {Theme} from '@emotion/react';
3-
import {css} from '@emotion/react';
4-
import styled from '@emotion/styled';
52

6-
import {Chevron} from 'sentry/components/chevron';
3+
import {Container, Flex} from '@sentry/scraps/layout';
4+
import {Text} from '@sentry/scraps/text';
5+
76
import type {LinkProps} from 'sentry/components/core/link';
87
import {Link} from 'sentry/components/core/link';
98
import GlobalSelectionLink from 'sentry/components/globalSelectionLink';
10-
import {space} from 'sentry/styles/space';
11-
12-
const BreadcrumbList = styled('nav')`
13-
display: flex;
14-
align-items: center;
15-
padding: ${space(1)} 0;
16-
`;
9+
import {IconChevron} from 'sentry/icons';
10+
import {trackAnalytics} from 'sentry/utils/analytics';
1711

1812
export interface Crumb {
1913
/**
2014
* Label of the crumb
2115
*/
22-
label: React.ReactNode;
23-
24-
/**
25-
* Component will try to come up with unique key, but you can provide your own
26-
* (used when mapping over crumbs)
27-
*/
28-
key?: string;
16+
label: NonNullable<React.ReactNode>;
2917

3018
/**
3119
* It will keep the page filter values (projects, environments, time) in the
@@ -39,103 +27,98 @@ export interface Crumb {
3927
to?: LinkProps['to'] | null;
4028
}
4129

42-
interface Props extends React.HTMLAttributes<HTMLDivElement> {
43-
/**
44-
* Array of crumbs that will be rendered
45-
*/
30+
interface BreadcrumbsProps extends React.HTMLAttributes<HTMLDivElement> {
4631
crumbs: Crumb[];
47-
48-
/**
49-
* As a general rule of thumb we don't want the last item to be link as it most likely
50-
* points to the same page we are currently on. This is by default false, so that
51-
* people don't have to check if crumb is last in the array and then manually
52-
* assign `to: null/undefined` when passing props to this component.
53-
*/
54-
linkLastItem?: boolean;
5532
}
5633

5734
/**
5835
* Page breadcrumbs used for navigation, not to be confused with sentry's event breadcrumbs
5936
*/
60-
export function Breadcrumbs({crumbs, linkLastItem = false, ...props}: Props) {
37+
export function Breadcrumbs({crumbs, ...props}: BreadcrumbsProps) {
6138
if (crumbs.length === 0) {
6239
return null;
6340
}
6441

65-
if (!linkLastItem) {
66-
crumbs[crumbs.length - 1]!.to = null;
67-
}
68-
6942
return (
70-
<BreadcrumbList {...props} data-test-id="breadcrumb-list">
43+
<Flex
44+
as="nav"
45+
gap="xs"
46+
align="center"
47+
padding="md 0"
48+
data-test-id="breadcrumb-list"
49+
{...props}
50+
>
7151
{crumbs.map((crumb, index) => {
72-
const {label, to, preservePageFilters, key} = crumb;
73-
const labelKey = typeof label === 'string' ? label : '';
74-
const mapKey =
75-
key ?? (typeof to === 'string' ? `${labelKey}${to}` : `${labelKey}${index}`);
76-
7752
return (
78-
<Fragment key={mapKey}>
79-
{to ? (
80-
<BreadcrumbLink
81-
to={to}
82-
preservePageFilters={preservePageFilters}
83-
data-test-id="breadcrumb-link"
84-
>
85-
{label}
86-
</BreadcrumbLink>
87-
) : (
88-
<BreadcrumbItem data-test-id="breadcrumb-item">{label}</BreadcrumbItem>
89-
)}
90-
91-
{index < crumbs.length - 1 && <BreadcrumbDividerIcon direction="right" />}
53+
<Fragment key={index}>
54+
<BreadCrumbItem
55+
crumb={{...crumb, to: index === crumbs.length - 1 ? undefined : crumb.to}}
56+
variant={index === crumbs.length - 1 ? 'primary' : 'muted'}
57+
/>
58+
{index < crumbs.length - 1 ? (
59+
<Flex align="center" justify="center" flexShrink={0}>
60+
<IconChevron size="xs" direction="right" color="subText" />
61+
</Flex>
62+
) : null}
9263
</Fragment>
9364
);
9465
})}
95-
</BreadcrumbList>
66+
</Flex>
9667
);
9768
}
9869

99-
const getBreadcrumbListItemStyles = (p: {theme: Theme}) => css`
100-
${p.theme.overflowEllipsis}
101-
color: ${p.theme.subText};
102-
width: auto;
70+
interface BreadCrumbItemProps {
71+
crumb: Crumb;
72+
variant: 'primary' | 'muted';
73+
}
10374

104-
&:last-child {
105-
color: ${p.theme.textColor};
75+
function BreadCrumbItem(props: BreadCrumbItemProps) {
76+
function onBreadcrumbLinkClick() {
77+
if (props.crumb.to) {
78+
trackAnalytics('breadcrumbs.link.clicked', {organization: null});
79+
}
10680
}
107-
`;
10881

109-
interface BreadcrumbLinkProps {
110-
to: LinkProps['to'];
82+
return (
83+
<Container maxWidth="400px" width="auto">
84+
{styleProps => {
85+
return props.crumb.to ? (
86+
<BreadcrumbLink
87+
to={props.crumb.to}
88+
preservePageFilters={props.crumb.preservePageFilters}
89+
data-test-id="breadcrumb-link"
90+
onClick={onBreadcrumbLinkClick}
91+
{...styleProps}
92+
>
93+
<Text ellipsis variant={props.variant}>
94+
{props.crumb.label}
95+
</Text>
96+
</BreadcrumbLink>
97+
) : (
98+
<Text
99+
ellipsis
100+
variant={props.variant}
101+
data-test-id="breadcrumb-item"
102+
{...styleProps}
103+
>
104+
{props.crumb.label}
105+
</Text>
106+
);
107+
}}
108+
</Container>
109+
);
110+
}
111+
112+
interface BreadcrumbLinkProps extends LinkProps {
111113
children?: React.ReactNode;
112114
preservePageFilters?: boolean;
113115
}
114116

115-
const BreadcrumbLink = styled(
116-
({preservePageFilters, to, ...props}: BreadcrumbLinkProps) =>
117-
preservePageFilters ? (
118-
<GlobalSelectionLink to={to} {...props} />
119-
) : (
120-
<Link to={to} {...props} />
121-
)
122-
)`
123-
${getBreadcrumbListItemStyles}
124-
max-width: 400px;
125-
126-
&:hover,
127-
&:active {
128-
color: ${p => p.theme.subText};
117+
function BreadcrumbLink(props: BreadcrumbLinkProps) {
118+
const {preservePageFilters, ...rest} = props;
119+
if (preservePageFilters) {
120+
return <GlobalSelectionLink {...rest} />;
129121
}
130-
`;
131122

132-
const BreadcrumbItem = styled('span')`
133-
${getBreadcrumbListItemStyles}
134-
max-width: 400px;
135-
`;
136-
137-
const BreadcrumbDividerIcon = styled(Chevron)`
138-
color: ${p => p.theme.subText};
139-
margin: 0 ${space(0.5)};
140-
flex-shrink: 0;
141-
`;
123+
return <Link {...rest} />;
124+
}

static/app/components/chevron.tsx

Lines changed: 0 additions & 90 deletions
This file was deleted.

static/app/utils/analytics.tsx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ import {
4040

4141
import type {AgentMonitoringEventParameters} from './analytics/agentMonitoringAnalyticsEvents';
4242
import {agentMonitoringEventMap} from './analytics/agentMonitoringAnalyticsEvents';
43+
import type {BreadcrumbsAnalyticsEventParameters} from './analytics/breadcrumbsAnalyticsEvents';
44+
import {breadcrumbsAnalyticsEventMap} from './analytics/breadcrumbsAnalyticsEvents';
4345
import type {CoreUIEventParameters} from './analytics/coreuiAnalyticsEvents';
4446
import {coreUIEventMap} from './analytics/coreuiAnalyticsEvents';
4547
import type {DashboardsEventParameters} from './analytics/dashboardsAnalyticsEvents';
@@ -98,6 +100,7 @@ interface EventParameters
98100
extends GrowthEventParameters,
99101
AgentMonitoringEventParameters,
100102
AlertsEventParameters,
103+
BreadcrumbsAnalyticsEventParameters,
101104
CoreUIEventParameters,
102105
DashboardsEventParameters,
103106
DiscoverEventParameters,
@@ -135,6 +138,7 @@ interface EventParameters
135138
const allEventMap: Record<string, string | null> = {
136139
...agentMonitoringEventMap,
137140
...alertsEventMap,
141+
...breadcrumbsAnalyticsEventMap,
138142
...coreUIEventMap,
139143
...dashboardsEventMap,
140144
...discoverEventMap,
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
export type BreadcrumbsAnalyticsEventParameters = {
2+
'breadcrumbs.link.clicked': {organization: null};
3+
'breadcrumbs.menu.opened': {organization: null};
4+
};
5+
6+
export const breadcrumbsAnalyticsEventMap: Record<
7+
keyof BreadcrumbsAnalyticsEventParameters,
8+
string | null
9+
> = {
10+
'breadcrumbs.link.clicked': 'Breadcrumbs: Link Clicked',
11+
'breadcrumbs.menu.opened': 'Breadcrumbs: Menu Opened',
12+
};

static/app/views/discover/landing.spec.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ describe('Discover > Landing', () => {
116116
</OrganizationContext>
117117
);
118118

119-
expect(await screen.findByText('Discover')).toHaveAttribute(
119+
expect(await screen.findByRole('link', {name: 'Discover'})).toHaveAttribute(
120120
'href',
121121
'/organizations/org-slug/explore/discover/homepage/'
122122
);

0 commit comments

Comments
 (0)