Stacked Nav #77665

natemoo-re · 2024-09-17T22:23:37Z

Update side navigation to new Stacked UI, behind the navigation-sidebar-v2 flag.

Rather than the approach taken in #76868, this PR implements the new navigation as an entirely new set of components in the components/nav/ directory. This new experience is rendered from the static/app/views/organizationLayout/index.tsx file. The intention is that the existing components/sidebar/ directory would be removed when this feature flag reaches GA.

Todo

Basic Tests
Basic Mobile nav

codecov · 2024-09-17T22:27:23Z

Bundle Report

Changes will increase total bundle size by 25.42kB (0.09%) ⬆️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
app-webpack-bundle-array-push	29.27MB	25.42kB (0.09%) ⬆️

natemoo-re · 2024-09-17T22:25:01Z

static/app/components/nav/config.tsx

+ };
+}
+
+export function useNavItems(): NavItemsResult {


The useNavItems hook acts as a single source of truth for the entire nav, including nested navigation and feature flag checks.

natemoo-re · 2024-09-17T22:28:18Z

static/app/views/organizationLayout/index.tsx

 // oganization is loaded before rendering children. Organization may not be
 // loaded yet when this first renders.
 const organization = useOrganization({allowNull: true});
+ const hasNavigationV2 = organization?.features.includes('navigation-sidebar-v2');
+ const App = hasNavigationV2 ? AppLayout : LegacyAppLayout;


Something to watch out for: this note calls out that "Organization may not be loaded yet when this first renders".

For this experience to exist behind a feature flag, the organization has to be loaded. This could potentially cause some layout thrashing once the org loads.

natemoo-re · 2024-09-17T22:30:28Z

static/app/views/organizationLayout/index.tsx

+ <AppContainer className="app">
+ <Nav />
+ <BodyContainer>
+ {organization && <OrganizationHeader organization={organization} />}
+ {organization && <DevToolInit />}
+ <Body>{children}</Body>
+ <Footer />
+ </BodyContainer>
+ </AppContainer>


Notable: adding a new wrapper for the main content allows us to remove the position: fixed CSS for the sidebar. This means that we no longer have to hardcode sidebar width and manually adjust padding for the content offset.

codecov · 2024-09-17T22:32:02Z

Codecov Report

Attention: Patch coverage is 52.68293% with 97 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
static/app/components/nav/mobile.tsx	6.45%	29 Missing ⚠️
static/app/components/nav/utils.tsx	53.44%	19 Missing and 8 partials ⚠️
static/app/components/nav/submenu.tsx	26.31%	14 Missing ⚠️
static/app/components/nav/useIndicator.tsx	38.09%	13 Missing ⚠️
static/app/components/nav/index.tsx	66.66%	5 Missing and 1 partial ⚠️
static/app/views/organizationLayout/index.tsx	55.55%	3 Missing and 1 partial ⚠️
static/app/views/issueList/utils.tsx	71.42%	1 Missing and 1 partial ⚠️
static/app/components/nav/config.tsx	94.44%	0 Missing and 1 partial ⚠️
static/app/components/nav/sidebar.tsx	95.83%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #77665      +/-   ##
==========================================
- Coverage   78.12%   78.10%   -0.02%     
==========================================
  Files        6996     7003       +7     
  Lines      310224   310429     +205     
  Branches    50762    50796      +34     
==========================================
+ Hits       242351   242461     +110     
- Misses      56142    56226      +84     
- Partials    11731    11742      +11

static/app/components/nav/index.tsx

JonasBa · 2024-09-21T14:43:43Z

static/app/components/nav/mobile.tsx

+ const [view, setView] = useState<'closed' | 'primary' | 'secondary'>('closed');
+ const handleClick = useCallback(() => {
+ setView(value => (value === 'closed' ? 'primary' : 'closed'));
+ }, [setView]);
+
+ const location = useLocation();
+
+ useEffect(() => {
+ setView('closed');
+ }, [location.pathname, setView]);
+
+ useEffect(() => {
+ const body = document.body;
+ const app = document.querySelector('.app > :not(nav)');
+ if (view !== 'closed') {
+ app?.setAttribute('inert', '');
+ body.style.setProperty('overflow', 'hidden');
+ } else {
+ app?.removeAttribute('inert');
+ body.style.removeProperty('overflow');
+ }
+ }, [view]);


It is generally preferable to use useLayoutEffect when doing such changes, useEffect might result in subtle flickering

You can get rid of this last useEffect if you call the body attrib logic inside the handler, it's generally preferred to do that as useEffects are annoying to follow

onViewChange = useCallback(view => { setView(view) updateNavStyleAttributes(view) // can be defined outside of the component },[setView]) useEffect... onViewChange('closed') [onViewChange]

Can we use a dedicated query selector and target the element directly (it would make it a bit more robust than having to rely on the dom selector). Right now if the selector fails, then logic will silently fail, but I think we want to log this as the nav should always be present?

JonasBa · 2024-09-21T14:50:47Z

static/app/components/nav/useIndicator.tsx

+ const handlePointerMove = (e: PointerEvent<HTMLUListElement>) => {
+ if (!(e.target instanceof Element)) return;
+ const a = e.target.closest('a');
+ const list = e.target.closest('ul');
+ if (!(a && list)) {
+ hover.current = null;
+ animation.current = {duration: 0.01};
+ return;
+ }
+ if (hover.current === a) return;
+ animation.current = {type: 'spring', mass: 0.1, damping: 10, stiffness: 150};
+ hover.current = a;
+ const newY = a?.getBoundingClientRect()?.y ?? 0;
+ const offset = list.getBoundingClientRect()?.y ?? 0;
+ setY(newY - offset);
+ };


This is going to be a performance problem (not sure to what extend), but we are triggering style and layout by calling getBoundingClientRect on both elements as well as using state to store the y variable. Since this will fire on pointer move, it will create a ton of updates

Is there a way you can do this without using state and calling bounding rects or using setState to track y coordinate?

static/app/components/nav/utils.tsx

JonasBa · 2024-09-21T15:25:40Z

static/app/components/nav/index.spec.tsx

+
+ mockUseLocation.mockReturnValue({...LocationFixture()});
+ });
+


Suggested change

afterEach(() => ConfigStore.reset())

I think you can also reset the store

JonasBa · 2024-09-21T15:27:57Z

static/app/components/nav/config.tsx

+export interface NavItemsResult {
+ primary: {
+ body: SidebarItem[];
+ footer: SidebarItem[];
+ };
+ secondary: {
+ body: SubmenuItem[];
+ footer: SubmenuItem[];
+ };
+}


Suggested change

export interface NavItemsResult {

primary: {

body: SidebarItem[];

footer: SidebarItem[];

};

secondary: {

body: SubmenuItem[];

footer: SubmenuItem[];

};

}

export interface NavItemsResult {

primary: {

body: ReadonlyArray<SidebarItem>;

footer: ReadonlyArray<SidebarItem>;

};

secondary: {

body: ReadonlyArray<SidebarItem>;

footer: ReadonlyArray<SidebarItem>;

};

}

I think you want to make sure these are not mutated in any component as it would impact the rendering

JonasBa · 2024-09-21T15:32:49Z

static/app/components/nav/utils.tsx

+
+interface NavItem {
+ label: string;
+ check?: FeatureCheck;


same comment about reusing the feature check signature if possible

JonasBa · 2024-09-21T15:33:10Z

static/app/components/nav/useIndicator.tsx

+ indicatorProps: HTMLAttributes<HTMLSpanElement>;
+}
+
+export function useIndicator() {


Suggested change

export function useIndicator() {

export function useNavIndicator() {

JonasBa · 2024-09-21T15:35:34Z

static/app/views/organizationLayout/index.tsx

@@ -33,20 +37,59 @@ function OrganizationLayout({children}: Props) {
 // oganization is loaded before rendering children. Organization may not be
 // loaded yet when this first renders.
 const organization = useOrganization({allowNull: true});
+ const hasNavigationV2 = organization?.features.includes('navigation-sidebar-v2');
+ const App = hasNavigationV2 ? AppLayout : LegacyAppLayout;


Thank you for not using V2 to name the layout as well :D

Co-authored-by: Jonas <jonas.badalic@sentry.io>

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Sep 17, 2024

natemoo-re commented Sep 17, 2024

View reviewed changes

natemoo-re mentioned this pull request Sep 17, 2024

Sidebar design update #76868

Closed

6 tasks

natemoo-re force-pushed the natemoo-re/nav-v2 branch from e6ab2eb to edfa094 Compare September 18, 2024 15:45

vercel bot deployed to Preview September 18, 2024 15:49 View deployment

vercel bot deployed to Preview September 18, 2024 19:59 View deployment

vercel bot deployed to Preview September 19, 2024 16:11 View deployment

natemoo-re force-pushed the natemoo-re/nav-v2 branch from 31cf655 to 70e3698 Compare September 19, 2024 16:13

vercel bot deployed to Preview September 19, 2024 16:17 View deployment

evanpurkhiser reviewed Sep 19, 2024

View reviewed changes

static/app/components/nav/index.tsx Outdated Show resolved Hide resolved

natemoo-re force-pushed the natemoo-re/nav-v2 branch from 7105bf5 to 73773de Compare September 19, 2024 22:12

vercel bot deployed to Preview September 19, 2024 22:18 View deployment

vercel bot deployed to Preview September 20, 2024 18:28 View deployment

vercel bot deployed to Preview September 20, 2024 22:03 View deployment

natemoo-re added 14 commits September 20, 2024 17:04

feat(nav): new nav components

e1c354a

feat(nav): update app layout

860d3ec

fix(nav): aria-current logic

28fa308

fix(nav): remove AnimatePresence

f3a8af4

feat(issues): add issue group consts

8e63a23

refactor(nav): support feature flagged nav items

9d1fe41

fix(nav): adjust insights display

8a86228

wip(nav): mobile layout

f7b3b5e

feat(nav): add top-level mobile menu

5c52f78

fix(nav): adjust mobile layout

5701297

fix(nav): issue group highlighting

4c6ca18

temp(nav): hardcode feature flag

b51b244

fix(nav): improve issue filter ux

c89a397

test(nav): add basic test

a773218

natemoo-re added 3 commits September 20, 2024 17:04

fix(nav): improve accessibility

6ff4019

fix(nav): revert hardcoded flag

070fd8b

tmp(nav): remove non-link footer items

7cc38b5

natemoo-re force-pushed the natemoo-re/nav-v2 branch from 7946948 to 7cc38b5 Compare September 20, 2024 22:05

chore(nav): add braces to if statement

ed6ac8b

natemoo-re marked this pull request as ready for review September 20, 2024 22:07

natemoo-re requested a review from a team as a code owner September 20, 2024 22:07

natemoo-re requested review from saponifi3d and JonasBa September 20, 2024 22:08

vercel bot deployed to Preview September 20, 2024 22:11 View deployment

test(nav): fix test assertion

75fe24a

vercel bot deployed to Preview September 20, 2024 22:52 View deployment

JonasBa reviewed Sep 21, 2024

View reviewed changes

refactor(nav): update util naming

bcf57ea

vercel bot deployed to Preview September 23, 2024 14:30 View deployment

natemoo-re and others added 2 commits September 23, 2024 09:32

refactor(nav): prefer pure function for location descriptor util

3bb7468

Update static/app/types/hooks.tsx

d41f3c1

Co-authored-by: Jonas <jonas.badalic@sentry.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stacked Nav #77665

Stacked Nav #77665

natemoo-re commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

natemoo-re Sep 17, 2024

natemoo-re Sep 17, 2024

natemoo-re Sep 17, 2024

codecov bot commented Sep 17, 2024 •

edited

Loading

JonasBa Sep 21, 2024

JonasBa Sep 21, 2024

JonasBa Sep 21, 2024

JonasBa Sep 21, 2024

JonasBa Sep 21, 2024

JonasBa Sep 21, 2024

JonasBa Sep 21, 2024

	export function useIndicator() {
	export function useNavIndicator() {

Stacked Nav #77665

Are you sure you want to change the base?

Stacked Nav #77665

Conversation

natemoo-re commented Sep 17, 2024 • edited Loading

Todo

codecov bot commented Sep 17, 2024 • edited Loading

Bundle Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemoo-re commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading