YPE-642 - verse action popover — highlights, copy & share #269
Conversation
…642) Tapping verses in BibleReader selects them (underline) and opens a popover anchored to the last-selected verse with 5 highlight colors, Copy, and Share. Highlights paint a 35%-opacity fill, persist to localStorage per Bible version (shaped like the future highlight API), and can be removed per-color. Copy/Share mirror bible.com formatting; Share uses the Web Share API with a clipboard fallback. Footnote markers darken on highlighted verses to stay AA-legible. Salvages the popover component and tests from the closed PR #131. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…642) Keep the verse action bar reachable while scrolling. An IntersectionObserver watches the anchored verse against the reader's scroll container; when the verse leaves the viewport the bar docks to the edge it exited through (scroll down → top, scroll up → bottom) instead of following the verse off-screen. A single Radix popover swaps its anchor between the real verse element and a virtual element pinned to the container edge, preserving focus/escape/outside-click and avoiding remount flicker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the popover closes, the parent clears the selection and anchor synchronously. Without freezing the last rendered state, the animating bar would jump to a fallback position and flash an empty layout before fading out.
Drop the unnecessary `typeof navigator` guards inside the Copy/Share click handlers (handlers run client-side only, matching verse-of-the-day's pattern), remove a no-op catch reassignment, and fix a stale dock-direction doc comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 3e7a7c1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
| function handleCopy() { | ||
| const text = buildSelectionText(); | ||
| if (text) void navigator.clipboard?.writeText(text); | ||
| closeAndClearSelection(); | ||
| } |
There was a problem hiding this comment.
nitpick: We may need the ability to pass in an override function for React Native Expo SDK.
|
|
||
| function handleShare() { | ||
| const text = buildSelectionText(); | ||
| if (typeof navigator !== 'undefined' && typeof navigator.share === 'function') { | ||
| navigator | ||
| .share({ text }) | ||
| .then(() => closeAndClearSelection()) | ||
| .catch(() => { | ||
| // Cancelled or failed — keep the popover open (AC 4). | ||
| }); | ||
| return; | ||
| } | ||
| // No Web Share support (e.g. most desktop browsers) — fall back to clipboard. | ||
| if (text && typeof navigator !== 'undefined') { | ||
| void navigator.clipboard?.writeText(text); | ||
| } | ||
| closeAndClearSelection(); | ||
| } |
There was a problem hiding this comment.
nitpick: We may need the ability to pass in an override function for React Native Expo SDK.
cameronapak
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Load this version's highlights when the version changes (client-only). | ||
| useEffect(() => { | ||
| let data: Record<string, string> = {}; | ||
| try { | ||
| const raw = localStorage.getItem(highlightsStorageKey); | ||
| if (raw) data = JSON.parse(raw) as Record<string, string>; | ||
| } catch { | ||
| // Ignore (unavailable or malformed storage). | ||
| } | ||
| setHighlightStore(data); | ||
| }, [highlightsStorageKey]); | ||
|
|
||
| // Navigating away (book/chapter/version) drops the selection — those verses no | ||
| // longer exist on screen (ADR-007). | ||
| useEffect(() => { | ||
| setSelectedVerses([]); | ||
| setPopoverOpen(false); | ||
| setAnchorElement(null); | ||
| lastSelectionRef.current = []; | ||
| }, [book, chapter, versionId]); |
There was a problem hiding this comment.
highlightStore renders for one frame when versionId changes
The load useEffect (line 483) and the ADR-007 navigation useEffect (line 496) are both async — they fire after the browser has painted. When the user switches Bible versions, versionId changes, the component re-renders with the new version's text, but highlightStore still holds the previous version's highlights until the load effect's setHighlightStore(data) call takes effect. During that first paint, the old version's passage IDs (${book}.${chapter}.N) match the new version's chapterPrefix, so the prior version's highlights flash visibly over the new text.
Adding setHighlightStore({}) as the first line inside the load useEffect ensures the store is cleared before the new data is applied, preventing the stale-data paint at the cost of one additional (empty) frame.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/bible-reader.tsx
Line: 482-501
Comment:
**Stale `highlightStore` renders for one frame when `versionId` changes**
The load `useEffect` (line 483) and the ADR-007 navigation `useEffect` (line 496) are both async — they fire after the browser has painted. When the user switches Bible versions, `versionId` changes, the component re-renders with the new version's text, but `highlightStore` still holds the previous version's highlights until the load effect's `setHighlightStore(data)` call takes effect. During that first paint, the old version's passage IDs (`${book}.${chapter}.N`) match the new version's `chapterPrefix`, so the prior version's highlights flash visibly over the new text.
Adding `setHighlightStore({})` as the first line inside the load `useEffect` ensures the store is cleared before the new data is applied, preventing the stale-data paint at the cost of one additional (empty) frame.
How can I resolve this? If you propose a fix, please make it concise.
| --- | ||
| "@youversion/platform-react-ui": minor | ||
| --- | ||
|
|
||
| Add a verse action popover to `BibleReader`. Tapping verses selects them (shown with an underline) and opens a popover anchored to the last-selected verse with five highlight colors, Copy, and Share. Highlights apply a translucent fill, persist to `localStorage` per Bible version (shaped like the future highlight API), and can be removed individually. Copy/Share output mirrors bible.com formatting: the verse text in curly quotes, gaps in a non-contiguous selection joined with `...`, followed by the `Book Chapter:verses VERSION` reference. Share uses the Web Share API and falls back to copying where it isn't available. |
There was a problem hiding this comment.
highlightedVerses type change is a breaking API change but the bump is minor
BibleTextViewProps.highlightedVerses changed from Record<number, boolean> to Record<number, string>. Any external consumer passing { 1: true } will get a TypeScript error after upgrading. The review guidelines require breaking changes to be clearly identified in changesets, and the project's unified-versioning rule means this bump propagates to all packages. The changeset should be major (or the ADR rationale — that the prop is wired nowhere in real usage today — should be noted explicitly in the changeset so consumers know it is safe to upgrade).
File Used: docs/review-guidelines.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: .changeset/verse-action-popover.md
Line: 1-5
Comment:
**`highlightedVerses` type change is a breaking API change but the bump is `minor`**
`BibleTextViewProps.highlightedVerses` changed from `Record<number, boolean>` to `Record<number, string>`. Any external consumer passing `{ 1: true }` will get a TypeScript error after upgrading. The review guidelines require breaking changes to be clearly identified in changesets, and the project's unified-versioning rule means this bump propagates to all packages. The changeset should be `major` (or the ADR rationale — that the prop is wired nowhere in real usage today — should be noted explicitly in the changeset so consumers know it is safe to upgrade).
**File Used:** docs/review-guidelines.md ([source](docs/review-guidelines.md))
How can I resolve this? If you propose a fix, please make it concise.1 participant
VIDEO OVERVIEW
YPE-642
Adds a verse action popover to
BibleReader: tap verse(s) to highlight, copy, or share.What's in it
Book Ch:vv VERSION. Non-contiguous picks join with...; ranges collapse (1-3). Share uses the Web Share API, clipboard fallback.bible_id+ verse USFM to mirror the futurehighlightAPI shape. No network this PR (server sync is a separate ticket).Behavior worth a look
Notes
--yv-muted-foregroundover a highlight for AA contrast.docs/adr/YPE-642-verse-action-popover.md.Testing
verse-action-popover+verse-sharesuites); typecheck + lint clean.🤖 Generated with Claude Code
Greptile Summary
This PR adds a verse action popover to
BibleReader: tapping one or more verses selects them (underline indicator), opens a floating toolbar anchored to the last-tapped verse, and exposes five highlight colors, Copy, and Share. Highlights persist tolocalStoragekeyed byversionIdand passage USFM, shaped to match the future API contract.verse-action-popover.tsx— new component with RadixvirtualRefanchoring, IntersectionObserver-based dock-to-edge behavior when the anchor verse scrolls off screen, and a frozen-snapshot pattern that lets the bar fade in place during close animations.verse-share.ts— new formatting helpers (formatVerseNumbers,joinVerseTexts,buildVerseShareText) implementing the bible.com output style (curly quotes,...gaps, collapsed verse ranges); 83 unit tests cover all edge cases.bible-reader.tsx/verse.tsx—Contentgains selection + highlight state, DOM-based anchor resolution, andgetCleanVerseTextfor clean prose extraction;highlightedVersestype widens fromRecord<number, boolean>toRecord<number, string>(hex).Confidence Score: 3/5
The core highlight, copy, and share flows work correctly, but an accessibility gap and a stale-highlight flash on version switches need to be resolved before this ships to all users.
All five color circle buttons share the same aria-label text ("Apply highlight" / "Clear highlight") — no color name is included — so a keyboard or screen reader user cannot distinguish them. This means the color selection part of the feature is effectively broken for AT users. Additionally, when a user switches Bible versions, the previous version's stored highlights appear on the new version's text for one render cycle because
highlightStoreis not cleared before the async load effect fires. The accessibility gap also has a test that inadvertently encodes the broken behavior, locking in the non-color-specific label.verse-action-popover.tsx(aria-label on color circles, prefers-reduced-motion) andbible-reader.tsx(stale highlight store during version switch). The changeset version bump also deserves a second look given thehighlightedVersestype change.Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant U as User participant BTV as BibleTextView participant C as Content (bible-reader) participant VAP as VerseActionPopover participant LS as localStorage U->>BTV: tap verse(s) BTV->>C: onVerseSelect(verses) C->>C: resolve anchor DOM element C->>VAP: "open=true, anchorElement, selectedVerses" VAP->>VAP: IntersectionObserver watches anchor Note over VAP: verse scrolls off screen VAP->>VAP: setDockEdge top or bottom U->>VAP: click color swatch (apply) VAP->>C: onHighlight(color) C->>LS: setItem(key, JSON) C->>C: closeAndClearSelection() U->>VAP: click X swatch (clear) VAP->>C: onClearHighlight(color) C->>LS: setItem(key, JSON) C->>C: if no remaining colors close U->>VAP: click Copy VAP->>C: onCopy() C->>C: getCleanVerseText x N C->>C: buildVerseShareText() C->>C: navigator.clipboard.writeText() U->>VAP: click Share VAP->>C: onShare() C->>C: buildVerseShareText() C->>C: navigator.share() fallback clipboard%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant U as User participant BTV as BibleTextView participant C as Content (bible-reader) participant VAP as VerseActionPopover participant LS as localStorage U->>BTV: tap verse(s) BTV->>C: onVerseSelect(verses) C->>C: resolve anchor DOM element C->>VAP: "open=true, anchorElement, selectedVerses" VAP->>VAP: IntersectionObserver watches anchor Note over VAP: verse scrolls off screen VAP->>VAP: setDockEdge top or bottom U->>VAP: click color swatch (apply) VAP->>C: onHighlight(color) C->>LS: setItem(key, JSON) C->>C: closeAndClearSelection() U->>VAP: click X swatch (clear) VAP->>C: onClearHighlight(color) C->>LS: setItem(key, JSON) C->>C: if no remaining colors close U->>VAP: click Copy VAP->>C: onCopy() C->>C: getCleanVerseText x N C->>C: buildVerseShareText() C->>C: navigator.clipboard.writeText() U->>VAP: click Share VAP->>C: onShare() C->>C: buildVerseShareText() C->>C: navigator.share() fallback clipboardComments Outside Diff (2)
packages/ui/src/components/verse-action-popover.tsx, line 1275-1282 (link)All five "apply" buttons receive the same label (
"Apply highlight") and all "clear" buttons receive the same label ("Clear highlight"). A screen reader user navigating the toolbar hears the same announcement for every color swatch and has no way to know which color they are about to apply or remove. This blocks the highlight-by-color feature for keyboard/AT users entirely.The i18n keys
applyHighlightAriaLabelandclearHighlightAriaLabelare also locked to a bare string in all three locale files, so there is no placeholder available for the color name. A fix requires adding a color-name map (e.g.{ fffe00: 'yellow', '5dff79': 'green', ... }) and updating the i18n keys to accept an interpolation variable (e.g."Apply {{color}} highlight"/"Clear {{color}} highlight").Prompt To Fix With AI
packages/ui/src/components/verse-action-popover.tsx, line 1240-1244 (link)prefers-reduced-motionguard on popover animationsThe popover uses
fade-in-0,zoom-in-95,slide-in-from-top-2, and related Tailwind animation utilities with a 180 ms custom duration. Users who prefer reduced motion (via the OS setting) will still see these transitions. The review guidelines call out respecting this media query for all animations.Adding
motion-safe:variants (e.g.yv:motion-safe:data-[state=open]:animate-in) or a global@media (prefers-reduced-motion: reduce)override that setsanimation-duration: 0son[data-yv-sdk]elements would resolve this.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor(ui): trim redundant guards and ..." | Re-trigger Greptile
Context used: