Skip to content

Adopt platform-bible-react components in place of raw HTML (Tier 2) - #174

Merged
alex-rawlings-yyc merged 1 commit into
mainfrom
165-adopt-platform-components-tier-2
Jul 27, 2026
Merged

Adopt platform-bible-react components in place of raw HTML (Tier 2)#174
alex-rawlings-yyc merged 1 commit into
mainfrom
165-adopt-platform-components-tier-2

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Resolves Tier 2 of #165.

What & why

platform-bible-react ships equivalents for elements the WebView hand-rolled as raw HTML. Migrating to the platform components gives visual/behavioral consistency with the host app, theming, and accessibility for free. Tier 1 (#168) covered the mechanical <button>/<input>/<textarea>/<label> swaps; this PR takes the one Tier 2 item that lands cleanly and records the verification result for the rest.

Changes (Tier 2)

WipeModal radio group → RadioGroup / RadioGroupItem (src/components/modals/WipeModal.tsx)

The destructive-wipe dialog's two <input type="radio"> options (current book / entire draft) are now a platform RadioGroup with RadioGroupItems, each paired with a Label. This gets us Radix's roving-tabindex arrow-key navigation, RTL support, and proper radio a11y semantics for free. Preserved verbatim:

  • data-testid="wipe-scope-book" / wipe-scope-all (unit tests and the wipeDraft e2e helper drive them by test id).
  • The disabled={!hasActiveBook} rule on the current-book option and the whole-draft default when no book is loaded.
  • Both option descriptions (tw:ps-6 tw:text-muted-foreground), still aligned under the option label.

onValueChange narrows to the WipeScope union without a cast (the repo's no-type-assertion rule forbids as).

Test mock — extended __mocks__/platform-bible-react.tsx with RadioGroup / RadioGroupItem stubs. The group shares its selected value + change handler via context; each item renders a native <input type="radio"> so jest-dom's toBeChecked / toBeDisabled matchers and userEvent.click work unchanged. All existing WipeModal tests pass without edits (6/6), and the full suite is green (1578/1578).

Deferred, with rationale — the content-sized gloss inputs

The issue gates the gloss-input migration (PhraseBox.tsx, MorphemeBox.tsx, TokenChip.tsx, SegmentFreeTranslationInput.tsx) on verifying that platform Input forwards className/style so field-sizing: content (and TokenChip's ARIA-combobox wiring) survives. Verified — and the answer is to keep these raw:

  • Platform Input does forward className (merged via cn), style, ARIA attributes, event handlers, and ref (React 19 ref-as-prop). So the forwarding itself is fine.
  • But Input also injects a full form-field baseline: tw:h-8 (fixed 2rem height), tw:rounded-lg, tw:border tw:border-input, tw:bg-transparent, tw:px-2.5 tw:py-1, tw:text-base, tw:min-w-0, focus-visible:ring-3, etc. The gloss inputs are the opposite — borderless-ish, text-xs/text-sm, px-1-tight, centered, content-sized inline editors styled by the custom @utility gloss-input.
  • Because gloss-input is a custom utility token, tailwind-merge can't recognize it as conflicting with the injected standard utilities, so it can't dedupe them. In particular the injected tw:h-8 sets an explicit height that overrides field-sizing: content's intrinsic height — the compact inline inputs would jump to 2rem tall and break interlinear row alignment.
  • Making Input look right would mean passing overrides to neutralize essentially all of its injected styling (h-auto, rounded, px-1, border-border, bg-background, …), which defeats the purpose of adopting it and adds fragility (future Input restyles would leak in). That's exactly the issue's stated non-goal: "Any element whose custom behavior can't be preserved on the platform component — keep raw and note why rather than regress UX."

SegmentFreeTranslationInput is the one of the four that isn't content-sized (it's a w-full bordered box), so it's the most migratable if the team later wants the host Input look there. I've left it with the group for now to avoid a lone restyle that would look inconsistent next to the raw compact gloss inputs in the same segment view.

The repo's AGENTS.md already documents this reserve-raw decision (Tier 1 added a "Components" note that names field-sizing: content gloss inputs as the canonical keep-raw example), so no doc change is needed here.

Tier 3 (ViewOptionsDropdownPopover; the SuggestionDropdown/TokenChip combobox) remains out of scope for this PR, per the issue's suggested sequencing.

Minor behavior note

In the old markup the entire option (title and description) was one <label>, so clicking the description text also selected the radio. With the shadcn-idiomatic RadioGroupItem + Label pattern (matching the platform's own RadioGroup story), the click target is the radio dot and its title label; the description span is not clickable. This mirrors the platform component's documented usage and is the only intentional UX change.

Drafted by Claude Opus 5


Devin: https://app.devin.ai/review/sillsdev/interlinearizer-extension/pull/174


This change is Reviewable

Summary by CodeRabbit

  • New Features
    • Added radio-button selection for choosing whether to wipe the current book or all content.
    • Preserved the disabled state for the book-only option when no active book is available.
    • Confirming the action continues to use the selected wipe scope.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e95f693c-157d-4c96-8bfb-3f64d5bf9881

📥 Commits

Reviewing files that changed from the base of the PR and between 1e59173 and 8926290.

📒 Files selected for processing (2)
  • __mocks__/platform-bible-react.tsx
  • src/components/modals/WipeModal.tsx

📝 Walkthrough

Walkthrough

WipeModal now uses RadioGroup and RadioGroupItem for wipe-scope selection, while the React test mock provides matching context-backed implementations.

Changes

Wipe scope radio-group

Layer / File(s) Summary
Radio-group mock implementation
__mocks__/platform-bible-react.tsx
Adds context-backed RadioGroup and RadioGroupItem test components and reformats the React import.
Wipe modal radio-group integration
src/components/modals/WipeModal.tsx
Replaces native radio markup with controlled radio-group components while preserving disabled state, labels, descriptions, test IDs, and confirmation scope handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • sillsdev/interlinearizer-extension#165 — Directly covers the WipeModal radio-group migration and corresponding mock updates.

Suggested reviewers: alex-rawlings-yyc

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: migrating WipeModal from raw HTML radios to platform-bible-react components.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 165-adopt-platform-components-tier-2

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Replace the hand-rolled two-radio group in WipeModal with platform
RadioGroup/RadioGroupItem + Label, preserving the wipe-scope-book /
wipe-scope-all test ids, the disabled-when-no-book rule, and the
scope descriptions. Extend the platform-bible-react jest mock with
RadioGroup/RadioGroupItem stubs.

The Tier 2 content-sized gloss inputs are intentionally left raw; see
the PR description for the verification that platform Input's injected
form-field styling can't preserve their field-sizing/compact layout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@imnasnainaec
imnasnainaec force-pushed the 165-adopt-platform-components-tier-2 branch from fb30289 to 8926290 Compare July 27, 2026 21:10
@imnasnainaec
imnasnainaec marked this pull request as ready for review July 27, 2026 21:33

@alex-rawlings-yyc alex-rawlings-yyc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@alex-rawlings-yyc reviewed 2 files and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).

@alex-rawlings-yyc
alex-rawlings-yyc enabled auto-merge (squash) July 27, 2026 21:59
@alex-rawlings-yyc
alex-rawlings-yyc merged commit 23bf3b3 into main Jul 27, 2026
10 checks passed
@alex-rawlings-yyc
alex-rawlings-yyc deleted the 165-adopt-platform-components-tier-2 branch July 27, 2026 21:59
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.

2 participants