From c4bc0ae48a812e7601ed2ac462b95e67fb0e322b Mon Sep 17 00:00:00 2001 From: Jacob Fletcher Date: Thu, 27 Feb 2025 15:21:28 -0500 Subject: [PATCH] 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 --- .../next/src/elements/Nav/index.client.tsx | 29 +++++----- packages/next/src/elements/Nav/index.scss | 52 ++++++++---------- packages/ui/src/elements/AppHeader/index.tsx | 8 +-- packages/ui/src/elements/Logout/index.tsx | 14 ++--- test/access-control/e2e.spec.ts | 5 +- test/access-control/payload-types.ts | 55 +++++++++++++++++++ test/admin/e2e/document-view/e2e.spec.ts | 2 +- test/admin/e2e/general/e2e.spec.ts | 11 +++- test/helpers.ts | 12 ---- test/helpers/e2e/toggleNav.ts | 21 +++++++ tsconfig.base.json | 2 +- 11 files changed, 138 insertions(+), 73 deletions(-) create mode 100644 test/helpers/e2e/toggleNav.ts diff --git a/packages/next/src/elements/Nav/index.client.tsx b/packages/next/src/elements/Nav/index.client.tsx index aeedc4957..1bb703545 100644 --- 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]) + const Label = ( + {getTranslation(label, i18n)} + ) + + if (activeCollection) { + return ( +
+
+ {Label} +
+ ) + } + return ( - - {activeCollection &&
} - {getTranslation(label, i18n)} - + + {Label} + ) })} diff --git a/packages/next/src/elements/Nav/index.scss b/packages/next/src/elements/Nav/index.scss index 93c03802f..636b6b103 100644 --- a/packages/next/src/elements/Nav/index.scss +++ b/packages/next/src/elements/Nav/index.scss @@ -93,36 +93,32 @@ } } - nav { - a { - 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) { - box-shadow: none; - 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; + position: relative; + padding-block: base(0.125); + padding-inline-start: 0; + padding-inline-end: base(1.5); + text-decoration: none; + + &:focus:not(:focus-visible) { + box-shadow: none; + font-weight: 600; + } + + &.active { + font-weight: normal; + padding-left: 0; + font-weight: 600; + } + } + + 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); } diff --git a/packages/ui/src/elements/AppHeader/index.tsx b/packages/ui/src/elements/AppHeader/index.tsx index 7307de39a..da539789b 100644 --- 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 && ( )} - } /> - +
diff --git a/packages/ui/src/elements/Logout/index.tsx b/packages/ui/src/elements/Logout/index.tsx index acde86663..2da58c732 100644 --- 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 ( diff --git a/test/access-control/e2e.spec.ts b/test/access-control/e2e.spec.ts index 006f94542..2ef3b566c 100644 --- 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() diff --git a/test/access-control/payload-types.ts b/test/access-control/payload-types.ts index ed98d4034..8be2a00c1 100644 --- 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; diff --git a/test/admin/e2e/document-view/e2e.spec.ts b/test/admin/e2e/document-view/e2e.spec.ts index 08dd76214..6dcdeeb12 100644 --- 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 import { navigateToDoc } from 'helpers/e2e/navigateToDoc.js' +import { openNav } from 'helpers/e2e/toggleNav.js' import path from 'path' import { fileURLToPath } from 'url' diff --git a/test/admin/e2e/general/e2e.spec.ts b/test/admin/e2e/general/e2e.spec.ts index 44139d192..6c84a828c 100644 --- 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 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() diff --git a/test/helpers.ts b/test/helpers.ts index 3ee90344e..4510cc382 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -255,18 +255,6 @@ export async function saveDocAndAssert( } } -export async function openNav(page: Page): Promise { - // 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 { await wait(500) // wait for parent form state to initialize await page.locator(selector).click() diff --git a/test/helpers/e2e/toggleNav.ts b/test/helpers/e2e/toggleNav.ts new file mode 100644 index 000000000..88108d5a9 --- /dev/null +++ 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 { + // 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() +} diff --git a/tsconfig.base.json b/tsconfig.base.json index fed3e543d..a12b54a3a 100644 --- 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"],