[P3] markers-checklist: follow-up - Sebastian UX-2 review (9 work packages)#2278
Open
rolfheij-sil wants to merge 11 commits into
Conversation
- #13: PostProcessParagraph no longer prepends `\marker` as a TextItem. The UI renders the marker from paragraph.Marker. INV-004 revised: Items contains only the original verse-text items (or empty when showVerseText=false). - #3 backend: PostProcessRows gains hasComparativeTexts parameter. The "Comparative texts have identical markers" Identical variant is now only emitted when at least one comparative is configured. The empty- with-no-comparatives case returns NoResults instead. - #3 frontend: empty-results fallback no longer defaults to the identical-markers string. New keys %markersChecklist_emptyResult_noResults% / _selectProject% drive a context-appropriate message. Co-Authored-By: Claude Code <noreply@anthropic.com>
… deferred) - #19: character-style items now render as <span className="usfm_{marker}"> with styling from a new checklist-usfm-styles.scss partial mirroring scripture-editor's _usj-nodes.scss character-style subset. The SCSS is imported via ?inline in checklist.web-view-provider.ts and concatenated with the tailwind styles passed to PAPI (the extension's webpack config uses sass-loader without style-loader, so the styles must be threaded through the web-view-provider's styles field rather than the standard side-effect import). No more literal "(\\nd Lord)". - #15: match rows get tw-bg-primary tw-text-primary-foreground on both the reference and project cells when row.isMatch === true. - #16: verse-number items in cells are now click-to-navigate buttons routed through onGotoLinkClick. The button's verseRef is reconstructed as `{book chapter}:{verseNumber}` from the owning cell's reference. Plain-superscript fallback preserved for read-only contexts. - #17: settings dialog focuses the equivalent-markers input on each open transition via requestAnimationFrame so Radix's focus trap is active before we focus. Replaces the autoFocus prop on the Input which fired before the focus trap mounted. - #18: deferred to be re-verified after WP4 cherry-picks shadcn-ui/command.tsx (which addresses the underlying cmdk Space propagation). Reopen if filter-dropdown space still scrolls the table after WP4. Adds Vitest regression tests for #19 (character-style className), #15 (match-row coloring on the reference cell), and #16 (verse-goto button wiring + verseRef reconstruction). Per CLAUDE.md the test file uses the `// @vitest-environment jsdom` directive because the extensions vitest config defaults to environment: 'node' and the new tests need RTL. Co-Authored-By: Claude Code <noreply@anthropic.com>
- #1: dropped the inner ChecklistTool TabToolbar. Selectors + view toggles now render inline in a single sticky row. The outer Platform tab chrome hamburger will host our menu items via WebViewMenu contributions (see WP6); the now-dead `projectMenuData` / `onSelectProjectMenuItem` props + clipboard/menu wiring have been removed to keep the web-view free of orphaned code. - #5: primary-project trigger no longer shows the truncating "Select primary S..." placeholder. The trigger renders the project's short name; the localized hint remains available via the aria-label / tooltip. Also dropped the redundant `primaryProjectName` lookup that was only used to populate the now-defunct placeholder. - #6: comparative-texts trigger shows a proper placeholder ("Select comparative texts") when empty. Added an opt-in `hideSelectAll` prop to ProjectSelector's `project-multi` mode and set it on the comparative trigger — selecting every project as a comparative is rarely useful and creates accidental wide queries. - #14: bidirectional same-project filter. Primary list hides every selected comparative; comparative list hides the primary. Filter logic extracted to `checklist-project-filter.utils.ts` with 7 unit tests so the rule can't regress silently. Co-Authored-By: Claude Code <noreply@anthropic.com>
Replace the ToggleGroup of Eye/EyeOff + Book/BookOpen buttons with a single eye-icon DropdownMenu containing checkbox items: - Hide matches (disabled when columnCount <= 1) - Show verse text Per UX-2 finding #8: the icon does not change with state; the checkbox marks inside the menu show the current state. Co-Authored-By: Claude Code <noreply@anthropic.com>
…toasts (#12) Hamburger menu rework per UX-2 finding #12: - Rename Copy -> "Copy all text to clipboard"; show Sonner success toast ("Checklist text copied to clipboard.") on successful clipboard write - Rename Settings -> "Markers Checklist Settings"; sync the dialog title (`markersChecklist_settings_title`) so menu + dialog match - Add "Print Markers Checklist" -- handler fires "not yet implemented" toast via NotificationService (`papi.notifications.send`) - Add "Save Markers Checklist" -- same not-implemented toast pattern - Add "Markers Inventory..." -- opens the existing `platformScripture.markersInventory` web view via the already- registered `platformScripture.openMarkersInventory` command Wiring details: The plan's original "intercept Copy in the web view" path no longer applies because WP3 deleted the inner TabToolbar. Menu items now dispatch through the outer Platform.Bible tab chrome, which calls `papi.commands.sendCommand(command, tabId)` -- so the web view never receives a direct `onSelectProjectMenuItem` callback. To preserve live `visibleData` access for the clipboard copy we introduce a companion broadcast network event (`CHECKLIST_COPY_REQUEST_EVENT`, mirroring the existing `CHECKLIST_OPEN_SETTINGS_EVENT`): the `copyMarkersChecklist` command in main.ts emits the event, and the web view subscribes via `useEvent` to build the clipboard text from the current `visibleData` snapshot. Clipboard write resilience: when the outer-chrome menu fires, focus sits on the main-frame menu, not the web view's iframe -- so the async clipboard API throws "Document is not focused". The handler calls `window.focus()` first and falls back to a hidden-textarea `execCommand('copy')` if the async path is still unreachable. All "not implemented" / "copy succeeded" messages use `papi.notifications.send({severity:'info'})` which surfaces as a non-blocking Sonner toast in the renderer's mounted <Toaster />. Verification: visual-verified via CDP -- menu order matches plan, all toasts render, clipboard receives table content, Markers Inventory opens, Settings dialog title updated. Lint clean, typecheck clean, 136/136 platform-scripture vitest tests pass, 706/706 C# tests pass. Co-Authored-By: Claude Code <noreply@anthropic.com>
Persist comparative projects, view toggles, equivalent markers, and marker filter to a user setting so a new checklist tab — or a fresh app session — inherits the user's last-committed defaults. - New setting: platformScripture.markersChecklistDefaults (declared in contributions/settings.json + SettingTypes augmentation in platform-scripture.d.ts). - New hook: useChecklistDefaults reads the setting on mount and exposes a writeDefaults callback that merges partial updates with the latest snapshot before calling papi.settings.set. Errors are swallowed + logged so a failed persist degrades to per-tab-only persistence. - Split checklist.web-view's body into an outer ChecklistWebView (reads the setting, gates rendering on isLoading) and an inner ChecklistContent (runs all the useWebViewState slots). The gate is required because useWebViewState's `useState(() => ...)` lazy init consults the default exactly once — without the gate, the first render would seed every slot with the static defaults and silently ignore the persisted value. - A new useEffect in ChecklistContent mirrors the persisted slots back to the setting on every change (last-write-wins for cross-tab races, accepted as a minor UX wart). - Scope and verse range are NOT persisted (matches PT9 memento behaviour and reviewer instruction). Cross-restart visual verification confirmed: comparative project + equivalentMarkers + markerFilter all survive `./.erb/scripts/refresh.sh` and seed a freshly-opened Markers Checklist tab. Tests: 6 new vitest cases in use-checklist-defaults.test.ts pin the hook contract (loading state, persisted-value exposure, partial-merge, read-fail fallback, write-merge, write-fail swallow). 173/173 platform- scripture tests pass; npm run typecheck + npm run lint clean. Co-Authored-By: Claude Code <noreply@anthropic.com>
- #10: enforce equal column widths via `table-fixed` layout + `tw-w-24` on the first `<th>` (Ref column). TanStack's DataTable does not honor `ColumnDef.size`, and inline widths on inner divs do not propagate to the `<th>` under table-fixed — widths must live on the `<th>` itself. Applied via Tailwind arbitrary variants on the `<section>` wrapper, no shared-lib changes. Replaces the previous auto-layout sizing where the widest cell drove the column width (making columns unequal). - #11: wrap the table section in `tw-relative tw-z-0` so the sticky thead's intrinsic `z-20` (set by shadcn-ui Table) stays inside a new stacking context and never out-stacks the toolbar (`z-10`). - #4: header tooltip + hover now cover the entire header cell (was just the text span). Cursor stays as the default arrow (neither pointer nor text). A subtle `bg-accent/50` on hover makes the hoverable area discoverable. Co-Authored-By: Claude Code <noreply@anthropic.com>
WP2's `tw-bg-primary tw-text-primary-foreground` was unreadable in both
light and dark modes — inner spans hardcode their own text colors
(`tw-text-foreground`, `tw-text-primary`, `tw-text-muted-foreground`),
overriding the outer container's `tw-text-primary-foreground`. Net
effect: same color on same color = invisible content. Additionally the
bg-color sat on the inner content div, so the colored region didn't
cover the full table cell (an unstyled border framed every match row).
Fix: replace with `tw-bg-primary/30` (subtle tint) and bleed the tint
to the cell edges via `-tw-m-4 tw-px-6 tw-py-6`, which cancels
`TableCell`'s built-in `tw-p-4` and restores the content offset. The
30% opacity lets the inner spans' default colors stay readable in
both light AND dark modes.
Approach A (negative-margin bleed on the cell's outermost element)
was chosen over modifying the shared `DataTable` to honor a
`columnDef.meta.className` (Approach B) because the shared DataTable
renders `<TableCell>{flexRender(cell, ctx)}</TableCell>` with no hook
for per-cell `<td>` classes — adding one would expand the PR scope to
`lib/platform-bible-react` and require ADR coverage.
- Drop `tw-text-primary-foreground` (no longer needed)
- Replace inner-div bg with outer-element bg that fills the full cell
- Update RTL tests to assert `tw-bg-primary/30` and the absence of
`tw-text-primary-foreground`
Co-Authored-By: Claude Code <noreply@anthropic.com>
…(#15) The first fix attempt (3b8b99b) tried to bleed a tw-bg-primary/30 inner container past TableCell's tw-p-4 with negative margins. Rolf reported the colored region still only covered the text/content area, not the full cell rectangle. This iteration applies the tint at the row level via a new getRowClassName prop on the shared DataTable component. The row's `<tr>` background paints under every `<td>`, so each cell truly fills edge-to-edge. Color is centralized in a single MATCH_ROW_BG_CLASS module-scope constant for one-place tuning. Changes: - lib/platform-bible-react/src/components/advanced/data-table: Add optional getRowClassName?: (row) => string | undefined prop. Forwarded to `<TableRow>`'s className. No behavior change when the prop is omitted. Dist rebuilt. - extensions/src/platform-scripture/src/components/checklist.component.tsx: MATCH_ROW_BG_CLASS = 'tw-bg-primary/20' (down from /30 — Rolf: too loud). Pass via getRowClassName when row.original.isMatch. Removed the negative- margin construction from both refColumn and projectColumns cells. - checklist.component.test.tsx: RTL tests now walk up from the reference cell to the `<tr>` and assert the bg class lives there, not on inner content. Co-Authored-By: Claude Code <noreply@anthropic.com>
Disable the outer Platform.Bible tab chrome for the markers checklist (shouldShowToolbar=false) and host the hamburger menu directly on the inner TabToolbar — partial reversal of WP3's "drop the inner toolbar" decision, prompted by Rolf's request to consolidate everything on one toolbar instead of duplicating chrome. Changes: - checklist.web-view-provider.ts: shouldShowToolbar false. - checklist.types.ts: restore the projectMenuData + onSelectProjectMenuItem props on ChecklistToolProps (with Localized + MultiColumnMenu imports). - checklist.component.tsx: restore the TabToolbar import and use it again as the toolbar wrapper, passing the project-menu props through. Wrap renderToolbarStart and renderToolbarEnd in `tw-flex tw-h-full tw-items-center` divs so selectors / view-menu trigger align horizontally with the hamburger (TabToolbar's inner area divs hardcode items-start, which top-aligned them while the hamburger center-aligned). - checklist.web-view.tsx: restore MARKERS_CHECKLIST_WEB_VIEW_TYPE + DEFAULT_WEBVIEW_MENU constants, the useData(papi.menuData...).WebViewMenu fetch + memoized narrowing, and the handleSelectProjectMenuItem dispatch (papi.commands.sendCommand for every item — no local Copy intercept, reuses WP6's CHECKLIST_COPY_REQUEST_EVENT plumbing unchanged). Menu items (Copy / Print / Save / Markers Inventory / Settings) still flow through the same registered commands in main.ts; only the dispatch origin moves from outer chrome to inner toolbar. Co-Authored-By: Claude Code <noreply@anthropic.com>
…th PR #2212 The earlier WP4 investigation concluded the Dropdown Variant was already on HEAD via PR #2219; that conclusion was incomplete. The shared component's JSX is present on HEAD, but four call-site / shared-lib gaps kept the markers-checklist's scope dropdown from matching the design in PR #2212's Dropdown Variant story: 1. The Navigate footer was never rendering — `checklist.web-view.tsx` passed `currentScrRef` but not `onCurrentScrRefChange`, and the component gates the footer on the callback. Fixed by wiring `onCurrentScrRefChange= {setLiveScrRef}` (option A — broadcasts via the scroll group; does NOT raise the editor tab, to keep focus inside the checklist). 2. Footer label read "Navigate" instead of "Change current reference". `localizedStrings.json` contributed "Navigate" for `webView_scope_selector_navigate`; renamed to "Change current reference" to match the design. 3. "Choose specific books" was missing from the dropdown because `availableScopes` excluded `selectedBooks`. Added the scope (option C from triage); `handleScopeChange` intercepts `selectedBooks` and fires a `papi.notifications.send` toast — "Choose specific books is not yet supported for the markers checklist." — without committing the scope change, so the previous scope stays active. The backend's ChecklistRequest.verseRange contract only supports contiguous start/end ranges, so a real implementation needs a separate follow-up. 4. Verse / Chapter / Book dropdown items weren't showing their scrRef suffixes (`MAT 5:3`, `MAT 5`, `MAT`). Root cause was in the shared ScopeSelector: `DropdownMenuContent`'s `tw-min-w-[12rem]` (192px) sat below the `DROPDOWN_NARROW_THRESHOLD_PX` (200), so `isDropdownNarrow` stayed true and suffixes were hidden. Bumped to `tw-min-w-[14rem]` (224px) so the content rect lands at ~216px (after shadcn's `tw-p-1`), clearing the threshold. Rebuilt platform-bible-react dist. Affects every ScopeSelector dropdown-variant consumer — 32px wider minimum, low risk. Files: - extensions/src/platform-scripture/src/checklist.web-view.tsx - extensions/src/platform-scripture/contributions/localizedStrings.json - lib/platform-bible-react/src/components/advanced/scope-selector/scope-selector.component.tsx - lib/platform-bible-react/dist/* (rebuilt) Co-Authored-By: Claude Code <noreply@anthropic.com>
rolfheij-sil
requested review from
irahopkinson,
jolierabideau,
lyonsil and
tjcouch-sil
as code owners
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up PR for the merged markers-checklist work (PR #2219). Addresses Sebastian's UX review delivered 2026-05-05 (
UX Review of Porting Result - Markers Checklist.csv): 22 findings + 4 improvement ideas (deferred per design).Design + implementation plan + perf-profile live in ai-prompts:
.context/features/markers-checklist/working-docs/ux-followup-design.md.context/features/markers-checklist/working-docs/ux-followup-implementation-plan.md.context/features/markers-checklist/working-docs/perf-profile.md(WP9)This is the Sebastian UX-2 follow-up; the TJ-review follow-up lives in PR #2254.
7 commits across 9 work packages
fdf9f176f66068e929058aa80f398fb686a2a81f07668b5acf91ce56a97be6d686f5b0Per-finding status (Sebastian's 22)
count("[aria-label='Project']") == 1.comparativeTextIds.Count > 0; UI now shows "Select a project to view its paragraph markers."tw-w-full tw-cursor-default hover:tw-bg-accent/50div wraps the cell; tooltip "rolf test" verified on hover.hideSelectAllprop.tw-relative tw-z-0so the toolbar's z-10 wins over the thead's z-20.\rem \rem/\toc1 \toc1duplicationPostProcessParagraph. Visual: each marker appears once.tw-bg-primary tw-text-primary-foregroundonisMatchrows. Visual verification fell back to unit tests because real match data was unreachable in the GEN 2:23 fixture.<button data-testid="checklist-verse-goto">wraps verse-number<sup>;onClickcallsonGotoVerseClick(verseRef).useEffect+requestAnimationFramefocusesequivalentMarkersInputRefafter the Radix focus trap activates. Code-verified; live verification difficult through CDP cross-iframe focus tracking.command.tsxSpace-handler)<span className="usfm_{characterStyle}" data-character-style="{characterStyle}">; CSS catalog lives inchecklist-usfm-styles.scss. Cells wrap content inchecklist-formatted-fontparent class.useChecklistDefaultsreads/writes user-settingplatformScripture.markersChecklistDefaults. Verified live: previously-selected 7 comparative texts and dialog values (p/q,q1 q2) reappear on reopen.Plan deviations
command.tsxSpace-handler. Findings #7 and Prepare release #18 were satisfied on HEAD without additional work; WP4 investigation confirmed and documented this.checklist.component.test.tsx.Known issues
lib/platform-bible-react/forDEFAULT_SCROLL_GROUP_LOCALIZED_STRINGS— these are unrelated to this PR and exist onai/main.import/no-named-as-defaultonProjectSelectorinlib/platform-bible-react/src/components/advanced/settings-components/settings-sidebar.component.tsx— unrelated.Tested
Live walkthrough against running app (CDP-attached headless Electron). Screenshots captured to
/tmp/wp-final-*.png:platformScripture.openMarkersChecklist.\q1 \q1duplication — Fix Lint warnings #13).tw-relative tw-z-0wrapper; toolbar z-10 wins (Add debugging to GHA workflows #11).Test plan
npm run lint— 0 errors (1 pre-existing warning, unrelated)npm run typecheck— cleannpm test— TS suites pass except the 5 pre-existing storybook failures noted abovedotnet testinc-sharp-tests/— 706 passed, 0 failed, 6 skipped🤖 Generated with Claude Code
This change is