From 08436a69221eb9aee07503145259a4e501d0a0e4 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 04:33:55 +0000 Subject: [PATCH] Reuse existing components and drop duplicated definitions Follow up on the drawer polish, replacing things it hand-rolled with what the codebase already provides. Reuse - The navigation toggles now use the Switch component instead of ~30 lines of CSS mirroring it, which carried a comment admitting the metrics were kept in sync by hand. base-ui renders Switch.Root as a span, so the row can be a label wrapping a real control, the same shape the params editor already uses. The row stays clickable across its full width - Remove scrollbar-width and scrollbar-color from three drawers. index.scss already sets both on the universal selector, so these were not only redundant, they overrode the app wide thumb and track tokens with local values - Collect the user facing view names in one map in viewerConfig, and derive the navigation entries and the URL preset targets from it. The same nine labels had been written out in three places Simplify - NavigationMenuToggle composes NavigationMenuItem rather than repeating its markup and key handling, so a row reacts to input in one place only - ViewParamsSection consulted collapsible five times in a render. Absence of a toggle handler now represents a section which cannot collapse, and the header owns its two shapes - Share the small uppercase group heading through an eyebrow-label mixin, which was copied across four stylesheets Tests - Cover that a collapsed section keeps its values through apply. The form reads from the DOM, so unmounting collapsed fields would silently drop them, and no existing test caught that Also drops a comment claiming rows avoid being buttons because Space is a global hotkey. Measured against the app, the hotkey does not fire while the menu is open regardless of focus, so the rationale did not hold. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014L7R2LgjMSH2WQbiSP4uRn --- .../NavigationMenu.module.scss | 8 +- .../navigation-menu/NavigationMenu.tsx | 6 +- .../NavigationMenuItem.module.scss | 40 ++------- .../NavigationMenuItem.tsx | 1 + .../NavigationMenuToggle.tsx | 29 ++---- .../OtherAddresses.module.scss | 5 +- .../ViewParamsEditor.module.scss | 3 - .../view-params-editor/ViewParamsEditor.tsx | 14 +-- .../ViewParamsPresets.module.scss | 7 +- .../ViewParamsSection.module.scss | 5 +- .../view-params-editor/ViewParamsSection.tsx | 88 +++++++++++-------- .../feature-panel/composite/URLPresetForm.tsx | 24 +++-- apps/client/src/theme/_mixins.scss | 8 ++ apps/client/src/viewerConfig.ts | 32 +++++-- e2e/tests/features/207-view-params.spec.ts | 23 +++++ 15 files changed, 141 insertions(+), 152 deletions(-) diff --git a/apps/client/src/common/components/navigation-menu/NavigationMenu.module.scss b/apps/client/src/common/components/navigation-menu/NavigationMenu.module.scss index cf0fb9661..4ecb643ec 100644 --- a/apps/client/src/common/components/navigation-menu/NavigationMenu.module.scss +++ b/apps/client/src/common/components/navigation-menu/NavigationMenu.module.scss @@ -79,9 +79,6 @@ flex: 1; overflow-y: auto; padding-block: 0.5rem 1rem; - - scrollbar-width: thin; - scrollbar-color: $gray-1000 transparent; } .group { @@ -95,10 +92,7 @@ .groupLabel { padding: 0.5rem 1.25rem 0.25rem; - font-size: $aux-text-size; - font-weight: 600; - letter-spacing: 0.06em; - text-transform: uppercase; + @include eyebrow-label; color: $gray-700; } diff --git a/apps/client/src/common/components/navigation-menu/NavigationMenu.tsx b/apps/client/src/common/components/navigation-menu/NavigationMenu.tsx index 48ca0eca1..83be1f065 100644 --- a/apps/client/src/common/components/navigation-menu/NavigationMenu.tsx +++ b/apps/client/src/common/components/navigation-menu/NavigationMenu.tsx @@ -64,20 +64,20 @@ function NavigationMenu({ isOpen, onClose }: NavigationMenuProps) {
{supportsFullscreen && ( - } label='Fullscreen' onClick={toggle} /> + } label='Fullscreen' onToggle={toggle} /> )} } label='Flip Screen' - onClick={() => toggleMirror()} + onToggle={() => toggleMirror()} /> {canUseWakeLock && ( } label='Keep Awake' - onClick={toggleKeepAwake} + onToggle={toggleKeepAwake} /> )} diff --git a/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.module.scss b/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.module.scss index 981c090f8..399a12009 100644 --- a/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.module.scss +++ b/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.module.scss @@ -45,6 +45,11 @@ background-color: $white-3; } + // toggle rows are labels, the focusable control sits inside them + &:focus-within { + background-color: $white-3; + } + &.current { background-color: rgba($blue-500, 0.16); border-left-color: $blue-400; @@ -62,38 +67,3 @@ min-width: 0; @include ellipsis-text; } - -/** - * Presentational switch for toggle rows. - * The row itself owns the interaction (role=switch), so this is aria-hidden: - * nesting a real control inside a clickable row would double up the hit target. - * Metrics and colours are kept in sync with the medium Switch component. - */ -.toggleTrack { - flex-shrink: 0; - box-sizing: content-box; - width: 2.5rem; - height: calc(1.5rem - 4px); - padding: 2px; - border-radius: $component-border-radius-full; - background-color: $gray-1100; - transition: background-color 125ms cubic-bezier(0.26, 0.75, 0.38, 0.45); - - &::after { - content: ''; - display: block; - width: calc(1.5rem - 4px); - height: 100%; - border-radius: $component-border-radius-full; - background-color: $ui-white; - transition: translate 150ms ease; - } -} - -[aria-checked='true'] > .toggleTrack { - background-color: $blue-700; - - &::after { - translate: calc(2.5rem - (1.5rem - 4px)) 0; - } -} diff --git a/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.tsx b/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.tsx index 789b3b99c..f35a67242 100644 --- a/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.tsx +++ b/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuItem.tsx @@ -11,6 +11,7 @@ interface NavigationMenuItemProps { onClick: () => void; } +/** A row in the navigation menu, and the single place which decides how a row reacts to input */ export default function NavigationMenuItem({ active, className, diff --git a/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuToggle.tsx b/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuToggle.tsx index 4538de7d3..64ff3f84c 100644 --- a/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuToggle.tsx +++ b/apps/client/src/common/components/navigation-menu/navigation-menu-item/NavigationMenuToggle.tsx @@ -1,6 +1,6 @@ import { ReactNode } from 'react'; -import { isKeyEnter } from '../../../utils/keyEvent'; +import Switch from '../../switch/Switch'; import style from './NavigationMenuItem.module.scss'; @@ -8,31 +8,16 @@ interface NavigationMenuToggleProps { checked: boolean; icon: ReactNode; label: string; - onClick: () => void; + onToggle: () => void; } -/** - * A menu row which reflects an on/off state. - * We keep the div + Enter handling of NavigationMenuItem instead of a button: - * Space is a global hotkey for toggling the menu, and a button would react to it as well. - */ -export default function NavigationMenuToggle({ checked, icon, label, onClick }: NavigationMenuToggleProps) { +/** A menu row which reflects, and toggles, an on/off state */ +export default function NavigationMenuToggle({ checked, icon, label, onToggle }: NavigationMenuToggleProps) { return ( -
{ - if (isKeyEnter(event)) { - onClick(); - } - }} - > +
+ + ); } diff --git a/apps/client/src/common/components/navigation-menu/other-addresses/OtherAddresses.module.scss b/apps/client/src/common/components/navigation-menu/other-addresses/OtherAddresses.module.scss index 02fe094cc..56f3c9b73 100644 --- a/apps/client/src/common/components/navigation-menu/other-addresses/OtherAddresses.module.scss +++ b/apps/client/src/common/components/navigation-menu/other-addresses/OtherAddresses.module.scss @@ -1,9 +1,6 @@ .header { padding: 0 1.25rem 0.25rem; - font-size: $aux-text-size; - font-weight: 600; - letter-spacing: 0.06em; - text-transform: uppercase; + @include eyebrow-label; color: $gray-700; } diff --git a/apps/client/src/common/components/view-params-editor/ViewParamsEditor.module.scss b/apps/client/src/common/components/view-params-editor/ViewParamsEditor.module.scss index b6262805e..e4fc1f1ff 100644 --- a/apps/client/src/common/components/view-params-editor/ViewParamsEditor.module.scss +++ b/apps/client/src/common/components/view-params-editor/ViewParamsEditor.module.scss @@ -79,9 +79,6 @@ flex: 1; padding: 1rem; overflow-y: auto; - - scrollbar-width: thin; - scrollbar-color: $gray-1000 transparent; } .footer { diff --git a/apps/client/src/common/components/view-params-editor/ViewParamsEditor.tsx b/apps/client/src/common/components/view-params-editor/ViewParamsEditor.tsx index 95068206f..7ab984f6d 100644 --- a/apps/client/src/common/components/view-params-editor/ViewParamsEditor.tsx +++ b/apps/client/src/common/components/view-params-editor/ViewParamsEditor.tsx @@ -4,6 +4,7 @@ import { FormEvent, memo } from 'react'; import { IoClose } from 'react-icons/io5'; import { useSearchParams } from 'react-router'; +import { viewLabels } from '../../../viewerConfig'; import useViewSettings from '../../hooks-query/useViewSettings'; import { useIsSmallScreen } from '../../hooks/useIsSmallScreen'; import { useSavedViewParams } from '../../stores/savedViewParams'; @@ -18,19 +19,6 @@ import ViewParamsSection from './ViewParamsSection'; import style from './ViewParamsEditor.module.scss'; -/** User facing names for the views we can customise */ -const viewLabels: Record = { - [OntimeView.Editor]: 'Editor', - [OntimeView.Cuesheet]: 'Cuesheet', - [OntimeView.Operator]: 'Operator', - [OntimeView.Timer]: 'Timer', - [OntimeView.Backstage]: 'Backstage', - [OntimeView.Timeline]: 'Timeline', - [OntimeView.StudioClock]: 'Studio Clock', - [OntimeView.Countdown]: 'Countdown', - [OntimeView.ProjectInfo]: 'Project Info', -}; - interface EditFormDrawerProps { target: OntimeView; viewOptions: ViewOption[]; diff --git a/apps/client/src/common/components/view-params-editor/ViewParamsPresets.module.scss b/apps/client/src/common/components/view-params-editor/ViewParamsPresets.module.scss index 4476c3538..f11447309 100644 --- a/apps/client/src/common/components/view-params-editor/ViewParamsPresets.module.scss +++ b/apps/client/src/common/components/view-params-editor/ViewParamsPresets.module.scss @@ -11,10 +11,7 @@ display: flex; align-items: center; - font-size: $aux-text-size; - font-weight: 600; - letter-spacing: 0.06em; - text-transform: uppercase; + @include eyebrow-label; color: $gray-300; } @@ -24,8 +21,6 @@ max-height: 10rem; overflow-y: auto; scrollbar-gutter: stable; - scrollbar-width: thin; - scrollbar-color: $gray-1000 transparent; } .preset { diff --git a/apps/client/src/common/components/view-params-editor/ViewParamsSection.module.scss b/apps/client/src/common/components/view-params-editor/ViewParamsSection.module.scss index 97c63d814..e2f4b467b 100644 --- a/apps/client/src/common/components/view-params-editor/ViewParamsSection.module.scss +++ b/apps/client/src/common/components/view-params-editor/ViewParamsSection.module.scss @@ -14,10 +14,7 @@ min-height: 2.75rem; padding: 0.5rem 1rem; - font-size: $aux-text-size; - font-weight: 600; - letter-spacing: 0.06em; - text-transform: uppercase; + @include eyebrow-label; color: $gray-300; &.collapsible { diff --git a/apps/client/src/common/components/view-params-editor/ViewParamsSection.tsx b/apps/client/src/common/components/view-params-editor/ViewParamsSection.tsx index 16a51da7e..68e1b1876 100644 --- a/apps/client/src/common/components/view-params-editor/ViewParamsSection.tsx +++ b/apps/client/src/common/components/view-params-editor/ViewParamsSection.tsx @@ -30,56 +30,66 @@ export default function ViewParamsSection({ title, collapsible, options }: ViewP return ; } - const handleCollapse = () => { - if (collapsible) { - setCollapsed((prev) => !prev); - } - }; - const isCollapsed = Boolean(collapsible && collapsed); - const isCustomised = options.some((option) => searchParams.has(option.id)); return (
-
{ - if (isKeyEnter(event)) { - handleCollapse(); - } - }} - role={collapsible ? 'button' : undefined} - tabIndex={collapsible ? 0 : undefined} - aria-expanded={collapsible ? !isCollapsed : undefined} - > - - {title} - {isCustomised && } - - {collapsible && } -
+ searchParams.has(option.id))} + collapsed={isCollapsed} + onToggle={collapsible ? () => setCollapsed((prev) => !prev) : undefined} + /> {/* collapsed options stay mounted: the form reads its values from the DOM */}
- -
-
- ); -} - -function SectionContents({ options }: { options: ParamField[] }) { - return ( - <> - {options.map((option) => { - return ( + {options.map((option) => ( - ); - })} - + ))} +
+ + ); +} + +interface SectionHeaderProps { + title: string; + isCustomised: boolean; + collapsed: boolean; + /** a header without a toggle belongs to a section which cannot collapse */ + onToggle?: () => void; +} + +function SectionHeader({ title, isCustomised, collapsed, onToggle }: SectionHeaderProps) { + const label = ( + + {title} + {isCustomised && } + + ); + + if (!onToggle) { + return
{label}
; + } + + return ( +
{ + if (isKeyEnter(event)) { + onToggle(); + } + }} + > + {label} + +
); } diff --git a/apps/client/src/features/app-settings/panel/feature-panel/composite/URLPresetForm.tsx b/apps/client/src/features/app-settings/panel/feature-panel/composite/URLPresetForm.tsx index 4eb5812b8..f1af3bf74 100644 --- a/apps/client/src/features/app-settings/panel/feature-panel/composite/URLPresetForm.tsx +++ b/apps/client/src/features/app-settings/panel/feature-panel/composite/URLPresetForm.tsx @@ -12,22 +12,28 @@ import { useUpdateUrlPreset } from '../../../../../common/hooks-query/useUrlPres import { isUrlSafe } from '../../../../../common/utils/regex'; import { enDash } from '../../../../../common/utils/styleUtils'; import { generateUrlPresetOptions } from '../../../../../common/utils/urlPresets'; +import { viewLabels } from '../../../../../viewerConfig'; import CuesheetLinkOptions, { CuesheetPermissionValues } from '../../../../sharing/composite/CuesheetLinkOptions'; import * as Panel from '../../../panel-utils/PanelUtils'; import style from './URLPresetForm.module.scss'; -const targetOptions: SelectOption[] = [ - { value: OntimeView.Cuesheet, label: 'Cuesheet' }, - { value: OntimeView.Operator, label: 'Operator' }, - { value: OntimeView.Timer, label: 'Timer' }, - { value: OntimeView.Backstage, label: 'Backstage' }, - { value: OntimeView.Timeline, label: 'Timeline' }, - { value: OntimeView.StudioClock, label: 'Studio Clock' }, - { value: OntimeView.Countdown, label: 'Countdown' }, - { value: OntimeView.ProjectInfo, label: 'Project Info' }, +const presettableTargets: OntimeViewPresettable[] = [ + OntimeView.Cuesheet, + OntimeView.Operator, + OntimeView.Timer, + OntimeView.Backstage, + OntimeView.Timeline, + OntimeView.StudioClock, + OntimeView.Countdown, + OntimeView.ProjectInfo, ]; +const targetOptions: SelectOption[] = presettableTargets.map((value) => ({ + value, + label: viewLabels[value], +})); + const formId = 'url-preset-form'; const defaultValues: URLPreset = { diff --git a/apps/client/src/theme/_mixins.scss b/apps/client/src/theme/_mixins.scss index 7e8b23fe5..2469b6c3b 100644 --- a/apps/client/src/theme/_mixins.scss +++ b/apps/client/src/theme/_mixins.scss @@ -8,6 +8,14 @@ text-overflow: ellipsis; } +/** Small uppercase heading used to label groups of items in drawers and lists */ +@mixin eyebrow-label { + font-size: $aux-text-size; + font-weight: 600; + letter-spacing: 0.06em; + text-transform: uppercase; +} + @mixin rotate-fourty-five { transform: rotate(45deg); } diff --git a/apps/client/src/viewerConfig.ts b/apps/client/src/viewerConfig.ts index 88388853b..2b9799df4 100644 --- a/apps/client/src/viewerConfig.ts +++ b/apps/client/src/viewerConfig.ts @@ -1,12 +1,30 @@ -export const navigatorConstants = [ - { url: 'timer', label: 'Timer' }, - { url: 'backstage', label: 'Backstage' }, - { url: 'timeline', label: 'Timeline' }, - { url: 'studio', label: 'Studio Clock' }, - { url: 'countdown', label: 'Countdown' }, - { url: 'info', label: 'Project Info' }, +import { OntimeView } from 'ontime-types'; + +/** User facing name of each view, the single source for view naming in the UI */ +export const viewLabels: Record = { + [OntimeView.Editor]: 'Editor', + [OntimeView.Cuesheet]: 'Cuesheet', + [OntimeView.Operator]: 'Operator', + [OntimeView.Timer]: 'Timer', + [OntimeView.Backstage]: 'Backstage', + [OntimeView.Timeline]: 'Timeline', + [OntimeView.StudioClock]: 'Studio Clock', + [OntimeView.Countdown]: 'Countdown', + [OntimeView.ProjectInfo]: 'Project Info', +}; + +/** Views offered in the navigation menu, in the order they are shown. Their value is also their route */ +const navigatorViews = [ + OntimeView.Timer, + OntimeView.Backstage, + OntimeView.Timeline, + OntimeView.StudioClock, + OntimeView.Countdown, + OntimeView.ProjectInfo, ]; +export const navigatorConstants = navigatorViews.map((view) => ({ url: view, label: viewLabels[view] })); + // default time format to use for users in 12 hour clocks export const FORMAT_12 = 'h:mm:ss a'; // default time format to use for users in 24 hour clocks diff --git a/e2e/tests/features/207-view-params.spec.ts b/e2e/tests/features/207-view-params.spec.ts index 8179f6ac3..1e82c5dac 100644 --- a/e2e/tests/features/207-view-params.spec.ts +++ b/e2e/tests/features/207-view-params.spec.ts @@ -13,3 +13,26 @@ test('View params configures timer view', async ({ page }) => { await expect(page.getByText('TIME NOW', { exact: true })).not.toBeInViewport(); await expect(page).toHaveURL(/.*hideClock=true/); }); + +/** + * The form gathers its values from the DOM, so a collapsed section has to stay mounted. + * Unmounting it would quietly drop everything the user set in it on the next apply. + */ +test('View params keeps the values of collapsed sections', async ({ page }) => { + // hideClock lives in the section we are about to collapse + await page.goto('/timer?hideClock=true'); + await expect(page.getByText('TIME NOW', { exact: true })).not.toBeInViewport(); + + await page.mouse.move(Math.random() * 100, Math.random() * 100); + await page.getByTestId('navigation__toggle-settings').click(); + + const section = page.getByRole('button', { name: /Element visibility/ }); + await expect(section).toHaveAttribute('aria-expanded', 'true'); + await section.click(); + await expect(section).toHaveAttribute('aria-expanded', 'false'); + + await page.getByTestId('apply-view-params').click(); + + await expect(page).toHaveURL(/.*hideClock=true/); + await expect(page.getByText('TIME NOW', { exact: true })).not.toBeInViewport(); +});