From 8a6ae88b37f1d900a4437494a245f2f4028d9199 Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Mon, 20 Jul 2026 15:06:44 -0500 Subject: [PATCH 1/2] feat(ui): checkmark on active highlight swatch --- .changeset/checkmark-active-swatch.md | 7 +++ docs/adr/YPE-642-verse-action-popover.md | 5 +++ packages/ui/src/components/icons/check.tsx | 19 ++++++++ .../components/verse-action-popover.test.tsx | 44 +++++++++++++++++++ .../src/components/verse-action-popover.tsx | 28 ++++++------ 5 files changed, 90 insertions(+), 13 deletions(-) create mode 100644 .changeset/checkmark-active-swatch.md create mode 100644 packages/ui/src/components/icons/check.tsx diff --git a/.changeset/checkmark-active-swatch.md b/.changeset/checkmark-active-swatch.md new file mode 100644 index 00000000..5df3f80e --- /dev/null +++ b/.changeset/checkmark-active-swatch.md @@ -0,0 +1,7 @@ +--- +'@youversion/platform-react-ui': patch +--- + +BibleReader's verse action popover now marks active swatches with a checkmark (YPE-1034 PR3, still behind the internal `HIGHLIGHTS_LIVE` flag). + +- **Checkmark swap**: active/remove swatches now render a 24px checkmark (`icons/check`) instead of the X, matching iOS (platform-sdk-swift #179). Same theme-invariant `#121212` fill and identical behavior — tapping still removes the highlight. diff --git a/docs/adr/YPE-642-verse-action-popover.md b/docs/adr/YPE-642-verse-action-popover.md index 97a2376b..eb522547 100644 --- a/docs/adr/YPE-642-verse-action-popover.md +++ b/docs/adr/YPE-642-verse-action-popover.md @@ -146,6 +146,11 @@ Selection is always enabled in BibleReader (no opt-out prop for now; YAGNI). - Tunable; swap to a ring/bg later if Figma says otherwise. ## As-built notes (deviations from the design above) +- **ADR-005 active-swatch icon (YPE-1034 PR3):** the 24px **X** on active/remove + swatches was replaced with a 24px **checkmark** (`icons/check`), matching iOS + (platform-sdk-swift #179). Same Text/Everdark (`--yv-gray-50` = `#121212`, + theme-invariant) fill and identical behavior — tapping still removes the + highlight; the swatch's `Clear highlight` aria-label is unchanged. - **ADR-004 revised:** selection + highlights live in `Content`, **not** Root context. Copy/Share/anchor all need the rendered verse DOM (which lives in Content), so Root ownership would fragment the feature. BibleTextView stays diff --git a/packages/ui/src/components/icons/check.tsx b/packages/ui/src/components/icons/check.tsx new file mode 100644 index 00000000..32bdeba2 --- /dev/null +++ b/packages/ui/src/components/icons/check.tsx @@ -0,0 +1,19 @@ +import type { ComponentProps, ReactElement } from 'react'; + +export function CheckIcon(props: ComponentProps<'svg'>): ReactElement { + return ( + + + + ); +} diff --git a/packages/ui/src/components/verse-action-popover.test.tsx b/packages/ui/src/components/verse-action-popover.test.tsx index db04c396..9a971b63 100644 --- a/packages/ui/src/components/verse-action-popover.test.tsx +++ b/packages/ui/src/components/verse-action-popover.test.tsx @@ -480,6 +480,50 @@ describe('VerseActionPopover', () => { .filter((btn) => btn.getAttribute('aria-label')?.includes('Clear')); } + describe('Active swatch checkmark', () => { + // The checkmark path (Swift #179) starts at these coords — asserts we render + // the check, not the old X. + const CHECK_PATH_PREFIX = 'M19.6627'; + + it('renders a checkmark (not an X) on the active/remove swatch', () => { + render( + ([HIGHLIGHT_COLORS[0]])} + selectedVerses={[1]} + highlightedVerses={{ 1: HIGHLIGHT_COLORS[0] }} + />, + ); + + const removeButton = screen.getByRole('button', { name: /Clear highlight/ }); + const path = removeButton.querySelector('svg path'); + expect(path?.getAttribute('d')).toContain(CHECK_PATH_PREFIX); + }); + + it('apply swatches render no icon', () => { + render(); + applyButtons().forEach((btn) => { + expect(btn.querySelector('svg')).toBeNull(); + }); + }); + + it('tapping the checkmark swatch still removes the highlight', () => { + const onClearHighlight = vi.fn(); + render( + ([HIGHLIGHT_COLORS[0]])} + selectedVerses={[1]} + highlightedVerses={{ 1: HIGHLIGHT_COLORS[0] }} + onClearHighlight={onClearHighlight} + />, + ); + + fireEvent.click(screen.getByRole('button', { name: /Clear highlight/ })); + expect(onClearHighlight).toHaveBeenCalledWith(HIGHLIGHT_COLORS[0]); + }); + }); + describe('Highlights disabled (flag off)', () => { it('hides the color row and remove circles but keeps Copy / Share', () => { render( diff --git a/packages/ui/src/components/verse-action-popover.tsx b/packages/ui/src/components/verse-action-popover.tsx index e8a30764..ac91bf1f 100644 --- a/packages/ui/src/components/verse-action-popover.tsx +++ b/packages/ui/src/components/verse-action-popover.tsx @@ -5,7 +5,7 @@ import i18n from '@/i18n'; import { cn } from '../lib/utils'; import { BoxStackIcon } from './icons/box-stack'; import { BoxArrowUpIcon } from './icons/box-arrow-up'; -import { XIcon } from './icons/x'; +import { CheckIcon } from './icons/check'; type Measurable = { getBoundingClientRect: () => DOMRect }; @@ -47,12 +47,12 @@ type VerseActionPopoverProps = { type ColorCircleProps = { color: string; - showX: boolean; + showRemove: boolean; label: string; onClick: () => void; }; -function ColorCircle({ color, showX, label, onClick }: ColorCircleProps) { +function ColorCircle({ color, showRemove, label, onClick }: ColorCircleProps) { return ( ); } @@ -201,10 +203,10 @@ export const VerseActionPopover: FC = ({ ? HIGHLIGHT_COLORS : HIGHLIGHT_COLORS.filter((c) => !activeHighlights.has(c)); - // X (remove) circles come first, then apply circles in canonical order. + // Remove (checkmark) circles come first, then apply circles in canonical order. const colorCircles = [ - ...activeColors.map((color) => ({ color, showX: true, key: `${color}-clear` })), - ...colorsToApply.map((color) => ({ color, showX: false, key: `${color}-apply` })), + ...activeColors.map((color) => ({ color, showRemove: true, key: `${color}-clear` })), + ...colorsToApply.map((color) => ({ color, showRemove: false, key: `${color}-apply` })), ]; // Snapshot of everything the Content renders. While open we keep it fresh; the @@ -291,13 +293,13 @@ export const VerseActionPopover: FC = ({ role="group" aria-label={t('highlightColorsAriaLabel')} > - {view.colorCircles.map(({ color, showX, key }) => ( + {view.colorCircles.map(({ color, showRemove, key }) => ( (showX ? onClearHighlight(color) : onHighlight(color))} + showRemove={showRemove} + label={showRemove ? t('clearHighlightAriaLabel') : t('applyHighlightAriaLabel')} + onClick={() => (showRemove ? onClearHighlight(color) : onHighlight(color))} /> ))} From 5fc51f7dc2babccbad3e8c53853f9cf940109e45 Mon Sep 17 00:00:00 2001 From: Cameron Pak Date: Wed, 22 Jul 2026 11:00:41 -0500 Subject: [PATCH 2/2] fix(ui): address PR #286 review nitpicks Add aria-hidden to CheckIcon and drop duplicate clear-highlight click test. Co-authored-by: Cursor --- packages/ui/src/components/icons/check.tsx | 1 + .../src/components/verse-action-popover.test.tsx | 16 ---------------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/packages/ui/src/components/icons/check.tsx b/packages/ui/src/components/icons/check.tsx index 32bdeba2..9f9faaa1 100644 --- a/packages/ui/src/components/icons/check.tsx +++ b/packages/ui/src/components/icons/check.tsx @@ -8,6 +8,7 @@ export function CheckIcon(props: ComponentProps<'svg'>): ReactElement { viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" + aria-hidden {...props} > { expect(btn.querySelector('svg')).toBeNull(); }); }); - - it('tapping the checkmark swatch still removes the highlight', () => { - const onClearHighlight = vi.fn(); - render( - ([HIGHLIGHT_COLORS[0]])} - selectedVerses={[1]} - highlightedVerses={{ 1: HIGHLIGHT_COLORS[0] }} - onClearHighlight={onClearHighlight} - />, - ); - - fireEvent.click(screen.getByRole('button', { name: /Clear highlight/ })); - expect(onClearHighlight).toHaveBeenCalledWith(HIGHLIGHT_COLORS[0]); - }); }); describe('Highlights disabled (flag off)', () => {