fix(next): disables active nav item (#11434)

When visiting a collection's list view, the nav item corresponding to that collection correctly appears in an active state, but is still rendered as an anchor tag. This makes it possible to reload the current page by simply clicking the link, which is a problem because this performs an unnecessary server roundtrip. This is especially apparent when search params exist in the current URL, as the href on the link does not. Unrelated: also cleans up leftover code that was missed in this PR: #11155
2025-02-27 15:21:28 -05:00
parent f7b1cd9d63
commit c4bc0ae48a
11 changed files with 138 additions and 73 deletions
--- a/packages/next/src/elements/Nav/index.client.tsx
+++ b/packages/next/src/elements/Nav/index.client.tsx
@@ -44,23 +44,26 @@ export const DefaultNavClient: React.FC<{
                id = `nav-global-${slug}`
              }

-              const LinkElement = Link || 'a'
              const activeCollection =
                pathname.startsWith(href) && ['/', undefined].includes(pathname[href.length])

-              return (
-                <LinkElement
-                  className={[`${baseClass}__link`, activeCollection && `active`]
-                    .filter(Boolean)
-                    .join(' ')}
-                  href={href}
-                  id={id}
-                  key={i}
-                  prefetch={Link ? false : undefined}
-                >
-                  {activeCollection && <div className={`${baseClass}__link-indicator`} />}
+              const Label = (
                <span className={`${baseClass}__link-label`}>{getTranslation(label, i18n)}</span>
-                </LinkElement>
+              )
+
+              if (activeCollection) {
+                return (
+                  <div className={`${baseClass}__link active`} id={id} key={i}>
+                    <div className={`${baseClass}__link-indicator`} />
+                    {Label}
+                  </div>
+                )
+              }
+
+              return (
+                <Link className={`${baseClass}__link`} href={href} id={id} key={i} prefetch={false}>
+                  {Label}
+                </Link>
              )
            })}
          </NavGroup>
--- a/packages/next/src/elements/Nav/index.scss
+++ b/packages/next/src/elements/Nav/index.scss
@@ -93,13 +93,13 @@
      }
    }

-    nav {
-      a {
+    &__link {
+      display: flex;
+      align-items: center;
      position: relative;
      padding-block: base(0.125);
      padding-inline-start: 0;
      padding-inline-end: base(1.5);
-        display: flex;
      text-decoration: none;

      &:focus:not(:focus-visible) {
@@ -107,22 +107,18 @@
        font-weight: 600;
      }

-        &:hover,
-        &:focus-visible {
-          text-decoration: underline;
-        }
-
      &.active {
        font-weight: normal;
        padding-left: 0;
        font-weight: 600;
      }
    }
-    }

-    &__link {
-      display: flex;
-      align-items: center;
+    a.nav__link {
+      &:hover,
+      &:focus-visible {
+        text-decoration: underline;
+      }
    }

    &__link-indicator {
@@ -148,7 +144,7 @@
        padding: var(--app-header-height) var(--gutter-h) base(2);
      }

-      nav a {
+      &__link {
        font-size: base(0.875);
        line-height: base(1.5);
      }
--- a/packages/ui/src/elements/AppHeader/index.tsx
+++ b/packages/ui/src/elements/AppHeader/index.tsx
@@ -59,8 +59,6 @@ export function AppHeader({ CustomAvatar, CustomIcon }: Props) {
    }
  }, [Actions])

-  const LinkElement = Link || 'a'
-
  const ActionComponents = Actions ? Object.values(Actions) : []

  return (
@@ -95,15 +93,15 @@ export function AppHeader({ CustomAvatar, CustomIcon }: Props) {
            {localization && (
              <LocalizerLabel ariaLabel="invisible" className={`${baseClass}__localizer-spacing`} />
            )}
-            <LinkElement
+            <Link
              aria-label={t('authentication:account')}
              className={`${baseClass}__account`}
              href={formatAdminURL({ adminRoute, path: accountRoute })}
-              prefetch={Link ? false : undefined}
+              prefetch={false}
              tabIndex={0}
            >
              <RenderCustomComponent CustomComponent={CustomAvatar} Fallback={<Account />} />
-            </LinkElement>
+            </Link>
          </div>
        </div>
      </div>
--- a/packages/ui/src/elements/Logout/index.tsx
+++ b/packages/ui/src/elements/Logout/index.tsx
@@ -28,21 +28,17 @@ export const Logout: React.FC<{
    routes: { admin: adminRoute },
  } = config

-  const props = {
-    'aria-label': t('authentication:logOut'),
-    className: `${baseClass}__log-out`,
-    prefetch: Link ? false : undefined,
-    tabIndex,
-    title: t('authentication:logOut'),
-  }
-
  return (
    <Link
-      {...props}
+      aria-label={t('authentication:logOut')}
+      className={`${baseClass}__log-out`}
      href={formatAdminURL({
        adminRoute,
        path: logoutRoute,
      })}
+      prefetch={false}
+      tabIndex={tabIndex}
+      title={t('authentication:logOut')}
    >
      <LogOutIcon />
    </Link>
--- a/test/access-control/e2e.spec.ts
+++ b/test/access-control/e2e.spec.ts
@@ -4,6 +4,7 @@ import type { TypeWithID } from 'payload'
 import { expect, test } from '@playwright/test'
 import { devUser } from 'credentials.js'
 import { openDocControls } from 'helpers/e2e/openDocControls.js'
+import { openNav } from 'helpers/e2e/toggleNav.js'
 import path from 'path'
 import { wait } from 'payload/shared'
 import { fileURLToPath } from 'url'
@@ -18,7 +19,6 @@ import {
  getRoutes,
  initPageConsoleErrorCatch,
  login,
-  openNav,
  saveDocAndAssert,
 } from '../helpers.js'
 import { AdminUrlUtil } from '../helpers/adminUrlUtil.js'
@@ -312,7 +312,7 @@ describe('Access Control', () => {
    test('should not show in nav', async () => {
      await page.goto(url.admin)
      await openNav(page)
-      // await expect(page.locator('.nav >> a:has-text("Restricteds")')).toHaveCount(0)
+
      await expect(
        page.locator('.nav a', {
          hasText: exactText('Restricteds'),
@@ -532,7 +532,6 @@ describe('Access Control', () => {
      test('should restrict access based on user settings', async () => {
        const url = `${serverURL}/admin/globals/settings`
        await page.goto(url)
-        await expect.poll(() => page.url(), { timeout: POLL_TOPASS_TIMEOUT }).toContain(url)
        await openNav(page)
        await expect(page.locator('#nav-global-settings')).toBeVisible()
        await expect(page.locator('#nav-global-test')).toBeHidden()
--- a/test/access-control/payload-types.ts
+++ b/test/access-control/payload-types.ts
@@ -6,11 +6,66 @@
 * and re-run `payload generate:types` to regenerate this file.
 */

+/**
+ * Supported timezones in IANA format.
+ *
+ * This interface was referenced by `Config`'s JSON-Schema
+ * via the `definition` "supportedTimezones".
+ */
+export type SupportedTimezones =
+  | 'Pacific/Midway'
+  | 'Pacific/Niue'
+  | 'Pacific/Honolulu'
+  | 'Pacific/Rarotonga'
+  | 'America/Anchorage'
+  | 'Pacific/Gambier'
+  | 'America/Los_Angeles'
+  | 'America/Tijuana'
+  | 'America/Denver'
+  | 'America/Phoenix'
+  | 'America/Chicago'
+  | 'America/Guatemala'
+  | 'America/New_York'
+  | 'America/Bogota'
+  | 'America/Caracas'
+  | 'America/Santiago'
+  | 'America/Buenos_Aires'
+  | 'America/Sao_Paulo'
+  | 'Atlantic/South_Georgia'
+  | 'Atlantic/Azores'
+  | 'Atlantic/Cape_Verde'
+  | 'Europe/London'
+  | 'Europe/Berlin'
+  | 'Africa/Lagos'
+  | 'Europe/Athens'
+  | 'Africa/Cairo'
+  | 'Europe/Moscow'
+  | 'Asia/Riyadh'
+  | 'Asia/Dubai'
+  | 'Asia/Baku'
+  | 'Asia/Karachi'
+  | 'Asia/Tashkent'
+  | 'Asia/Calcutta'
+  | 'Asia/Dhaka'
+  | 'Asia/Almaty'
+  | 'Asia/Jakarta'
+  | 'Asia/Bangkok'
+  | 'Asia/Shanghai'
+  | 'Asia/Singapore'
+  | 'Asia/Tokyo'
+  | 'Asia/Seoul'
+  | 'Australia/Sydney'
+  | 'Pacific/Guam'
+  | 'Pacific/Noumea'
+  | 'Pacific/Auckland'
+  | 'Pacific/Fiji';
+
 export interface Config {
  auth: {
    users: UserAuthOperations;
    'public-users': PublicUserAuthOperations;
  };
+  blocks: {};
  collections: {
    users: User;
    'public-users': PublicUser;
--- a/test/admin/e2e/document-view/e2e.spec.ts
+++ b/test/admin/e2e/document-view/e2e.spec.ts
@@ -11,7 +11,6 @@ import {
  ensureCompilationIsDone,
  exactText,
  initPageConsoleErrorCatch,
-  openNav,
  saveDocAndAssert,
 } from '../../../helpers.js'
 import { AdminUrlUtil } from '../../../helpers/adminUrlUtil.js'
@@ -46,6 +45,7 @@ const description = 'Description'
 let payload: PayloadTestSDK<Config>

 import { navigateToDoc } from 'helpers/e2e/navigateToDoc.js'
+import { openNav } from 'helpers/e2e/toggleNav.js'
 import path from 'path'
 import { fileURLToPath } from 'url'

--- a/test/admin/e2e/general/e2e.spec.ts
+++ b/test/admin/e2e/general/e2e.spec.ts
@@ -9,7 +9,6 @@ import {
  exactText,
  getRoutes,
  initPageConsoleErrorCatch,
-  openNav,
  saveDocAndAssert,
  saveDocHotkeyAndAssert,
  // throttleTest,
@@ -51,6 +50,7 @@ let payload: PayloadTestSDK<Config>

 import { navigateToDoc } from 'helpers/e2e/navigateToDoc.js'
 import { openDocControls } from 'helpers/e2e/openDocControls.js'
+import { openNav } from 'helpers/e2e/toggleNav.js'
 import path from 'path'
 import { fileURLToPath } from 'url'

@@ -406,6 +406,15 @@ describe('General', () => {
      await expect(link).toBeHidden()
    })

+    test('should disable active nav item', async () => {
+      await page.goto(postsUrl.list)
+      await openNav(page)
+      const activeItem = page.locator('.nav .nav__link.active')
+      await expect(activeItem).toBeVisible()
+      const tagName = await activeItem.evaluate((el) => el.tagName.toLowerCase())
+      expect(tagName).toBe('div')
+    })
+
    test('breadcrumbs — should navigate from list to dashboard', async () => {
      await page.goto(postsUrl.list)
      await page.locator(`.step-nav a[href="${adminRoutes.routes.admin}"]`).click()
--- a/test/helpers.ts
+++ b/test/helpers.ts
@@ -255,18 +255,6 @@ export async function saveDocAndAssert(
  }
 }

-export async function openNav(page: Page): Promise<void> {
-  // check to see if the nav is already open and if not, open it
-  // use the `--nav-open` modifier class to check if the nav is open
-  // this will prevent clicking nav links that are bleeding off the screen
-  if (await page.locator('.template-default.template-default--nav-open').isVisible()) {
-    return
-  }
-  // playwright: get first element with .nav-toggler which is VISIBLE (not hidden), could be 2 elements with .nav-toggler on mobile and desktop but only one is visible
-  await page.locator('.nav-toggler >> visible=true').click()
-  await expect(page.locator('.template-default.template-default--nav-open')).toBeVisible()
-}
-
 export async function openDocDrawer(page: Page, selector: string): Promise<void> {
  await wait(500) // wait for parent form state to initialize
  await page.locator(selector).click()
--- a/test/helpers/e2e/toggleNav.ts
+++ b/test/helpers/e2e/toggleNav.ts
@@ -0,0 +1,21 @@
+import type { Page } from '@playwright/test'
+
+import { expect } from '@playwright/test'
+import { wait } from 'payload/shared'
+
+export async function openNav(page: Page): Promise<void> {
+  // wait for the preferences/media queries to either open or close the nav
+  // TODO: remove this in the future when nav state can be initialized properly
+  await wait(250)
+
+  // check to see if the nav is already open and if not, open it
+  // use the `--nav-open` modifier class to check if the nav is open
+  // this will prevent clicking nav links that are bleeding off the screen
+  if (await page.locator('.template-default.template-default--nav-open').isVisible()) {
+    return
+  }
+
+  // playwright: get first element with .nav-toggler which is VISIBLE (not hidden), could be 2 elements with .nav-toggler on mobile and desktop but only one is visible
+  await page.locator('.nav-toggler >> visible=true').click()
+  await expect(page.locator('.template-default.template-default--nav-open')).toBeVisible()
+}
--- a/tsconfig.base.json
+++ b/tsconfig.base.json
@@ -31,7 +31,7 @@
      }
    ],
    "paths": {
-      "@payload-config": ["./test/fields-relationship/config.ts"],
+      "@payload-config": ["./test/access-control/config.ts"],
      "@payloadcms/live-preview": ["./packages/live-preview/src"],
      "@payloadcms/live-preview-react": ["./packages/live-preview-react/src/index.ts"],
      "@payloadcms/live-preview-vue": ["./packages/live-preview-vue/src/index.ts"],