Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(nav): Display secondary nav in mobile menu #84582

Merged
merged 3 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions static/app/components/nav/constants.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import {PrimaryNavGroup} from 'sentry/components/nav/types';
import {t} from 'sentry/locale';

export const NAV_SIDEBAR_COLLAPSED_LOCAL_STORAGE_KEY = 'navigation-sidebar-is-collapsed';

export const NAV_GROUP_LABELS: Record<PrimaryNavGroup, string> = {
[PrimaryNavGroup.ISSUES]: t('Issues'),
[PrimaryNavGroup.EXPLORE]: t('Explore'),
[PrimaryNavGroup.DASHBOARDS]: t('Dashboards'),
[PrimaryNavGroup.INSIGHTS]: t('Insights'),
[PrimaryNavGroup.SETTINGS]: t('Settings'),
};
36 changes: 34 additions & 2 deletions static/app/components/nav/context.tsx
Original file line number Diff line number Diff line change
@@ -1,25 +1,57 @@
import {createContext, useContext, useMemo, useState} from 'react';
import {useTheme} from '@emotion/react';

import {NAV_SIDEBAR_COLLAPSED_LOCAL_STORAGE_KEY} from 'sentry/components/nav/constants';
import {NavLayout, type PrimaryNavGroup} from 'sentry/components/nav/types';
import {useLocalStorageState} from 'sentry/utils/useLocalStorageState';
import useMedia from 'sentry/utils/useMedia';

export interface NavContext {
activeGroup: PrimaryNavGroup | null;
isCollapsed: boolean;
layout: NavLayout;
secondaryNavEl: HTMLElement | null;
setActiveGroup: (group: PrimaryNavGroup | null) => void;
setIsCollapsed: (isCollapsed: boolean) => void;
setSecondaryNavEl: (el: HTMLElement | null) => void;
}

const NavContext = createContext<NavContext>({
secondaryNavEl: null,
setSecondaryNavEl: () => {},
layout: NavLayout.SIDEBAR,
isCollapsed: false,
setIsCollapsed: () => {},
activeGroup: null,
setActiveGroup: () => {},
});

export function useNavContext(): NavContext {
return useContext(NavContext);
}

export function NavContextProvider({children}: {children: React.ReactNode}) {
const [isCollapsed, setIsCollapsed] = useLocalStorageState(
NAV_SIDEBAR_COLLAPSED_LOCAL_STORAGE_KEY,
false
);
const [secondaryNavEl, setSecondaryNavEl] = useState<HTMLElement | null>(null);
const [activeGroup, setActiveGroup] = useState<PrimaryNavGroup | null>(null);

const theme = useTheme();
const isMobile = useMedia(`(max-width: ${theme.breakpoints.medium})`);

const value = useMemo(
() => ({secondaryNavEl, setSecondaryNavEl}),
[secondaryNavEl, setSecondaryNavEl]
() => ({
secondaryNavEl,
setSecondaryNavEl,
layout: isMobile ? NavLayout.MOBILE : NavLayout.SIDEBAR,
isCollapsed,
setIsCollapsed,
activeGroup,
setActiveGroup,
}),
[secondaryNavEl, isMobile, isCollapsed, setIsCollapsed, activeGroup]
);
Comment on lines 44 to 55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is useMemo here doing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always wrap context values in useMemo to prevent every consumer of the context from rerendering if this component does. Though since the context is near the top of the render tree it probably doesn't matter much


return <NavContext.Provider value={value}>{children}</NavContext.Provider>;
Expand Down
54 changes: 53 additions & 1 deletion static/app/components/nav/index.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {render, screen, userEvent, within} from 'sentry-test/reactTestingLibrary
import Nav from 'sentry/components/nav';
import {NavContextProvider} from 'sentry/components/nav/context';
import {SecondaryNav} from 'sentry/components/nav/secondary';
import {PrimaryNavGroup} from 'sentry/components/nav/types';

const ALL_AVAILABLE_FEATURES = [
'insights-entry-points',
Expand All @@ -39,11 +40,12 @@ describe('Nav', function () {
render(
<NavContextProvider>
<Nav />
<SecondaryNav>
<SecondaryNav group={PrimaryNavGroup.ISSUES}>
<SecondaryNav.Item to="/organizations/org-slug/issues/foo/">
Foo
</SecondaryNav.Item>
</SecondaryNav>
<div id="main" />
</NavContextProvider>,
{
organization: OrganizationFixture({features: ALL_AVAILABLE_FEATURES}),
Expand Down Expand Up @@ -107,6 +109,56 @@ describe('Nav', function () {
expect(link).toHaveAttribute('aria-selected', 'true');
});
});

describe('mobile navigation', function () {
beforeEach(() => {
// Need useMedia() to return true for isMobile query
window.matchMedia = jest.fn().mockImplementation(query => ({
matches: true,
media: query,
onchange: null,
addListener: jest.fn(),
removeListener: jest.fn(),
addEventListener: jest.fn(),
removeEventListener: jest.fn(),
dispatchEvent: jest.fn(),
}));
});

it('renders mobile navigation on small screen sizes', async function () {
renderNav();

// Should have a top-level header element with a home link and menu button
expect(screen.getByRole('link', {name: 'Sentry Home'})).toBeInTheDocument();
expect(screen.getByRole('button', {name: 'Open main menu'})).toBeInTheDocument();

await userEvent.click(screen.getByRole('button', {name: 'Open main menu'}));

// Should first render the active secondary navigation
expect(
await screen.findByRole('navigation', {name: 'Secondary Navigation'})
).toBeInTheDocument();
expect(screen.getByRole('link', {name: 'Foo'})).toBeInTheDocument();

// Clicking back should render the primary navigation
await userEvent.click(
screen.getByRole('button', {name: 'Back to primary navigation'})
);
expect(
screen.getByRole('navigation', {name: 'Primary Navigation'})
).toBeInTheDocument();
expect(screen.getByRole('link', {name: 'Issues'})).toBeInTheDocument();
expect(screen.getByRole('link', {name: 'Explore'})).toBeInTheDocument();
expect(screen.getByRole('link', {name: 'Boards'})).toBeInTheDocument();
expect(screen.getByRole('link', {name: 'Insights'})).toBeInTheDocument();
expect(screen.getByRole('link', {name: 'Stats'})).toBeInTheDocument();
expect(screen.getByRole('link', {name: 'Settings'})).toBeInTheDocument();

// Tapping one of the primary navigation items should close the menu
await userEvent.click(screen.getByRole('link', {name: 'Explore'}));
expect(screen.queryByRole('navigation')).not.toBeInTheDocument();
});
});
});

describe('analytics', function () {
Expand Down
11 changes: 8 additions & 3 deletions static/app/components/nav/index.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import styled from '@emotion/styled';

import {useNavContext} from 'sentry/components/nav/context';
import MobileTopbar from 'sentry/components/nav/mobileTopbar';
import {Sidebar} from 'sentry/components/nav/sidebar';
import {useBreakpoints} from 'sentry/utils/metrics/useBreakpoints';
import {NavLayout} from 'sentry/components/nav/types';

function Nav() {
const screen = useBreakpoints();
const {layout} = useNavContext();

return <NavContainer>{screen.medium ? <Sidebar /> : <MobileTopbar />}</NavContainer>;
return (
<NavContainer>
{layout === NavLayout.SIDEBAR ? <Sidebar /> : <MobileTopbar />}
</NavContainer>
);
}

const NavContainer = styled('div')`
Expand Down
100 changes: 63 additions & 37 deletions static/app/components/nav/mobileTopbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,25 @@ import {useCallback, useEffect, useLayoutEffect, useState} from 'react';
import {createPortal} from 'react-dom';
import styled from '@emotion/styled';

import {Button} from 'sentry/components/button';
import Link from 'sentry/components/links/link';
import {useNavContext} from 'sentry/components/nav/context';
import {PrimaryNavigationItems} from 'sentry/components/nav/primary';
import {SecondaryMobile} from 'sentry/components/nav/secondaryMobile';
import {IconClose, IconMenu, IconSentry} from 'sentry/icons';
import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import theme from 'sentry/utils/theme';
import {useLocation} from 'sentry/utils/useLocation';
import useOrganization from 'sentry/utils/useOrganization';

type NavView = 'primary' | 'secondary' | 'closed';
type ActiveView = 'primary' | 'secondary' | 'closed';

function MobileTopbar() {
const {activeGroup} = useNavContext();
const location = useLocation();
const [view, setView] = useState<NavView>('closed');
const organization = useOrganization();
const [view, setView] = useState<ActiveView>('closed');
/** Sync menu state with `body` attributes */
useLayoutEffect(() => {
updateNavStyleAttributes(view);
Expand All @@ -22,21 +30,33 @@ function MobileTopbar() {
setView('closed');
}, [location.pathname]);
const handleClick = useCallback(() => {
setView(v => (v === 'closed' ? 'primary' : 'closed'));
}, [setView]);
setView(v => (v === 'closed' ? (activeGroup ? 'secondary' : 'primary') : 'closed'));
}, [activeGroup]);

return (
<Topbar>
<a href="/">
<HomeLink
to={`/organizations/${organization.slug}/issues/`}
aria-label={t('Sentry Home')}
>
<IconSentry />
</a>
<button onClick={handleClick}>
{view === 'closed' ? <IconMenu width={16} /> : <IconClose width={16} />}
</button>
</HomeLink>
<MenuButton
onClick={handleClick}
icon={view === 'closed' ? <IconMenu /> : <IconClose />}
aria-label={view === 'closed' ? t('Open main menu') : t('Close main menu')}
size="sm"
borderless
/>
{view !== 'closed' ? (
<OverlayPortal>
<PrimaryNavigationItems />
</OverlayPortal>
<NavigationOverlayPortal
label={view === 'primary' ? t('Primary Navigation') : t('Secondary Navigation')}
>
{view === 'primary' ? <PrimaryNavigationItems /> : null}
{view === 'secondary' ? (
<SecondaryMobile handleClickBack={() => setView('primary')} />
) : null}
</NavigationOverlayPortal>
) : null}
</Topbar>
);
Expand All @@ -45,7 +65,7 @@ function MobileTopbar() {
export default MobileTopbar;

/** When the mobile menu opens, set the main content to `inert` and disable `body` scrolling */
function updateNavStyleAttributes(view: NavView) {
function updateNavStyleAttributes(view: ActiveView) {
const mainContent = document.getElementById('main');
if (!mainContent) {
throw new Error(
Expand All @@ -62,11 +82,20 @@ function updateNavStyleAttributes(view: NavView) {
}
}

function OverlayPortal({children}: any) {
return createPortal(<Overlay>{children}</Overlay>, document.body);
function NavigationOverlayPortal({
children,
label,
}: {
children: React.ReactNode;
label: string;
}) {
return createPortal(
<NavigationOverlay aria-label={label}>{children}</NavigationOverlay>,
document.body
);
}

const Topbar = styled('div')`
const Topbar = styled('header')`
height: 40px;
width: 100vw;
padding: ${space(0.5)} ${space(1.5)} ${space(0.5)} ${space(1)};
Expand All @@ -80,34 +109,31 @@ const Topbar = styled('div')`
position: sticky;
top: 0;
z-index: ${theme.zIndex.sidebar};
`;

const HomeLink = styled(Link)`
display: flex;
align-items: center;
justify-content: center;
padding: 0 ${space(2)};
margin: -${space(1)};

svg {
display: block;
width: var(--size);
height: var(--size);
color: currentColor;
color: ${p => p.theme.white};
width: ${space(3)};
height: ${space(3)};
}
button {
all: initial;
--size: ${space(2)};
}
a {
--size: ${space(3)};
}
a,
button {
color: rgba(255, 255, 255, 0.85);
padding: ${space(1)};
margin: -${space(1)};
cursor: pointer;
`;

const MenuButton = styled(Button)`
color: ${p => p.theme.white};

&:hover {
color: white;
}
&:hover {
color: ${p => p.theme.white};
}
`;

const Overlay = styled('div')`
const NavigationOverlay = styled('nav')`
position: fixed;
top: 40px;
right: 0;
Expand Down
2 changes: 1 addition & 1 deletion static/app/components/nav/primary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ const SidebarItemWrapper = styled('li')`
height: 40px;
gap: ${space(1.5)};
align-items: center;
padding: auto ${space(1.5)};
padding: 0 ${space(1.5)};
color: var(--color, currentColor);
font-size: ${p => p.theme.fontSizeMedium};
font-weight: ${p => p.theme.fontWeightNormal};
Expand Down
3 changes: 2 additions & 1 deletion static/app/components/nav/secondary.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import NegativeSpaceContainer from 'sentry/components/container/negativeSpaceCon
import {NavContextProvider} from 'sentry/components/nav/context';
import {SecondaryNav} from 'sentry/components/nav/secondary';
import {SecondarySidebar} from 'sentry/components/nav/secondarySidebar';
import {PrimaryNavGroup} from 'sentry/components/nav/types';
import storyBook from 'sentry/stories/storyBook';
import {space} from 'sentry/styles/space';

Expand All @@ -16,7 +17,7 @@ export default storyBook('SecondaryNav', story => {
<Container>
<NavContextProvider>
<SecondarySidebar />
<SecondaryNav>
<SecondaryNav group={PrimaryNavGroup.ISSUES}>
<SecondaryNav.Body>
<SecondaryNav.Section>
<SecondaryNav.Item
Expand Down
Loading
Loading