Skip to content

Checkmark marks the active highlight swatch (YPE-1034) (3/4)#286

Merged
cameronapak merged 3 commits into
mainfrom
claude/ype-1034-pr3-recent-colors
Jul 22, 2026
Merged

Checkmark marks the active highlight swatch (YPE-1034) (3/4)#286
cameronapak merged 3 commits into
mainfrom
claude/ype-1034-pr3-recent-colors

Conversation

@cameronapak

@cameronapak cameronapak commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

The active highlight swatch renders a 24px checkmark (matching iOS) instead of an X. This PR's original recent-colors feature was dropped from the SDK entirely — the /v1/highlights/recent-colors contract is still changing and must not ship (per bbelyeu; main removed the API in #291) — leaving the checkmark as the surviving slice.

Stack: 1/4 #283 → 2/4 #2853/4 (this PR) → 4/4 #294 · restored from #288

Changes

  1. Active and remove swatches render a CheckIcon (24px, iOS-matching path) instead of the X.
  2. ADR-005 records the active-swatch icon decision.

Test plan

  • pnpm build && pnpm typecheck && pnpm lint && pnpm test at this branch: all green (core 315 + checkmark suite; ui 258 / hooks 284).
  • verse-action-popover.test.tsx asserts the checkmark SVG path on active/remove swatches.

Greptile Summary

Replaces the X icon on active highlight swatches with a 24 px checkmark, matching iOS parity (platform-sdk-swift #179). Behavior is unchanged — tapping a checkmark swatch still removes the highlight; only the indicator icon changes.

  • packages/ui/src/components/icons/check.tsx: new CheckIcon with aria-hidden default and {...props} spread, consistent with other icon files.
  • packages/ui/src/components/verse-action-popover.tsx: swaps XIconCheckIcon on active swatches; renames the showX prop to showRemove for clarity.
  • Tests, changeset, and ADR note are all included and accurate.

Confidence Score: 5/5

Safe to merge — the change is a pure icon swap with no behavioral or API surface changes.

The change is minimal and well-contained: a new icon component, a single prop rename, and two targeted tests that directly verify the checkmark path. Behavior (highlight removal on tap, aria-labels, flag gating) is untouched. The new icon correctly sets aria-hidden before the props spread, changeset and ADR are both present, and no new i18n strings or public API exports were added.

No files require special attention.

Important Files Changed

Filename Overview
packages/ui/src/components/icons/check.tsx New CheckIcon SVG component with aria-hidden default and props spread — follows existing icon conventions; improves on XIcon which lacked aria-hidden.
packages/ui/src/components/verse-action-popover.tsx Swaps XIcon → CheckIcon on active swatches; renames prop showX → showRemove for semantic accuracy; no behavior change.
packages/ui/src/components/verse-action-popover.test.tsx Adds two focused tests: one asserts the checkmark SVG path prefix on a remove swatch; one asserts apply swatches render no SVG. Both are correct.
.changeset/checkmark-active-swatch.md Patch changeset for @youversion/platform-react-ui; correctly scoped and descriptive.
docs/adr/YPE-642-verse-action-popover.md ADR-005 entry added documenting the X-to-checkmark swap with iOS parity rationale; no issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    VAP[VerseActionPopover] --> CC[ColorCircle]
    CC -->|showRemove = true\nactive highlight| CHK[CheckIcon\n24px checkmark]
    CC -->|showRemove = false\napply highlight| EMPTY[no icon]
    CHK -->|onClick| CLR[onClearHighlight]
    EMPTY -->|onClick| APP[onHighlight]
    CHK -->|aria-hidden=true| A11Y[Screen reader ignores icon\nbutton's aria-label carries meaning]
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into claude/ype-1034..." | Re-trigger Greptile

@changeset-bot

changeset-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d384cbe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@youversion/platform-react-ui Patch
vite-react Patch
@youversion/platform-core Patch
@youversion/platform-react-hooks Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cameronapak

Copy link
Copy Markdown
Collaborator Author

Superseded by #288, which consolidates the full YPE-1034 stack into a single PR rebased onto main.

@cameronapak cameronapak reopened this Jul 20, 2026
@cameronapak
cameronapak force-pushed the claude/ype-1034-pr2-highlight-auth-flow branch from f275bbf to 879e04e Compare July 20, 2026 20:15
@cameronapak
cameronapak force-pushed the claude/ype-1034-pr3-recent-colors branch from eef219b to 2f47c5f Compare July 20, 2026 20:15
@cameronapak cameronapak changed the title feat(ui): recent highlight colors + checkmark active swatch (YPE-1034, PR 3/3) Checkmark marks the active highlight swatch (YPE-1034) (3/4) Jul 20, 2026
@cameronapak
cameronapak marked this pull request as ready for review July 20, 2026 20:18
Comment thread packages/ui/src/components/icons/check.tsx
@cameronapak
cameronapak force-pushed the claude/ype-1034-pr2-highlight-auth-flow branch 4 times, most recently from 10f47c6 to 65a2ab2 Compare July 21, 2026 17:07
Base automatically changed from claude/ype-1034-pr2-highlight-auth-flow to main July 21, 2026 19:34
@cameronapak
cameronapak force-pushed the claude/ype-1034-pr3-recent-colors branch from 2f47c5f to 8a6ae88 Compare July 21, 2026 19:46
Comment thread packages/ui/src/components/verse-action-popover.test.tsx Outdated
@Dustin-Kelley

Copy link
Copy Markdown
Collaborator

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

Dustin-Kelley
Dustin-Kelley previously approved these changes Jul 22, 2026

@Dustin-Kelley Dustin-Kelley left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just address the nitpicks and you good 👊

Add aria-hidden to CheckIcon and drop duplicate clear-highlight click test.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cameronapak
cameronapak merged commit 04f79ee into main Jul 22, 2026
8 checks passed
@cameronapak
cameronapak deleted the claude/ype-1034-pr3-recent-colors branch July 22, 2026 16:07
cameronapak added a commit that referenced this pull request Jul 22, 2026
Take aria-hidden from main; keep this branch's checkmark-tap remove test.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants