WIP: YPE-2197: Extract standalone popover content components for native sheet rendering#222
WIP: YPE-2197: Extract standalone popover content components for native sheet rendering#222cameronapak wants to merge 1 commit intomainfrom
Conversation
…ick callbacks for native sheet rendering Extract BibleChapterPickerContent, BibleVersionPickerContent, BibleReaderSettingsContent, and FootnoteContent as standalone exported components. Add onContentClick/onFootnoteContentClick callbacks to intercept popover clicks for rendering in native bottom sheets via Expo DOM components. - BibleChapterPickerContent: self-contained, calls useBooks internally, manages search/accordion state - BibleVersionPickerContent: self-contained, calls useVersions/useLanguages internally, manages search/filter/language state - BibleReaderSettingsContent: exported with font size/family controls - FootnoteContent: renders footnote list with verse context, onFootnoteContentClick threaded through BibleTextView → Verse.Html → BibleTextHtml → VerseFootnoteButton - Added UserMenu to BibleReader compound export - All compound components support onContentClick prop to skip popover rendering - Added openCount key to force fresh mount on version picker reopen
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (searchQuery.trim() === '') return books.data; | ||
| return books.data.filter((book) => | ||
| book.title?.toLowerCase().includes(searchQuery.toLowerCase()), | ||
| ); | ||
| }, [books?.data, searchQuery]); |
There was a problem hiding this comment.
background prop declared but silently ignored
BibleChapterPickerContentProps exposes a background prop, but the component only destructures versionId, defaultBook, and onSelectChapter — background is never applied inside the JSX. The same issue exists in BibleVersionPickerContent (only destructures versionId / onSelectVersion) and BibleReaderSettingsContent (only destructures font-size/family state). When a consumer renders these in a native bottom sheet and passes background="dark", no theming takes effect — the component will always render without a theme attribute on its wrapper.
Consider either adding a data-yv-theme wrapper div keyed to background, or removing the prop from the public type until it's implemented.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/components/bible-chapter-picker.tsx
Line: 41-45
Comment:
**`background` prop declared but silently ignored**
`BibleChapterPickerContentProps` exposes a `background` prop, but the component only destructures `versionId`, `defaultBook`, and `onSelectChapter` — `background` is never applied inside the JSX. The same issue exists in `BibleVersionPickerContent` (only destructures `versionId` / `onSelectVersion`) and `BibleReaderSettingsContent` (only destructures font-size/family state). When a consumer renders these in a native bottom sheet and passes `background="dark"`, no theming takes effect — the component will always render without a theme attribute on its wrapper.
Consider either adding a `data-yv-theme` wrapper div keyed to `background`, or removing the prop from the public type until it's implemented.
How can I resolve this? If you propose a fix, please make it concise.
cameronapak
changed the title
1 participant
Summary
BibleChapterPickerContent,BibleVersionPickerContent,BibleReaderSettingsContent, andFootnoteContentas standalone exported components that can be rendered outside of popovers (e.g., in native bottom sheets via Expo DOM components)onContentClickcallbacks toBibleChapterPicker.Root,BibleVersionPicker.Root, andBibleReader.Rootthat intercept trigger clicks and pass content props to the callback instead of opening a popoveronFootnoteContentClickprop threaded throughBibleTextView→Verse.Html→BibleTextHtml→VerseFootnoteButtonUserMenutoBibleReadercompound exportTest plan
pnpm typecheckpassespnpm lintpassesopenCountkey)Greptile Summary
This PR extracts
BibleChapterPickerContent,BibleVersionPickerContent,BibleReaderSettingsContent, andFootnoteContentas standalone exported components so they can be rendered outside of popovers (e.g., in Expo DOM native bottom sheets). AnonContentClickcallback pattern is added to eachRootcomponent that intercepts trigger clicks and passes content props to the caller instead of opening a popover.backgroundprop is part of the public API for all three new content components but is never applied inside the component implementations — consumers usingbackground=\"dark\"in a native sheet will see no theming effect.BibleChapterPicker.TriggerandBibleVersionPicker.Triggerreplace<PopoverTrigger>with a<div onClick>whenonContentClickis set, breaking keyboard accessibility.Confidence Score: 3/5
Safe to merge after resolving the background theming gap in the standalone content components; accessibility issues should also be addressed.
A P1 finding (background prop part of public API but silently no-ops in all three content components) could mislead consumers of the new standalone API, particularly for the stated native-sheet use-case. Multiple P2 accessibility issues (div-based triggers) reinforce the need for a follow-up. Score sits at 3 to reflect the P1 API correctness gap.
packages/ui/src/components/bible-chapter-picker.tsx and packages/ui/src/components/bible-version-picker.tsx — background prop and keyboard-accessibility on the new div triggers.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Root["Root (onContentClick?)"] Root -->|"onContentClick provided"| CtxOnly["Context.Provider only\n(no Popover wrapper)"] Root -->|"no onContentClick"| PopoverRoot["Popover + Context.Provider"] CtxOnly --> Trigger PopoverRoot --> Trigger PopoverRoot --> Content["Compound Content\n(PopoverContent wrapper)"] Trigger{"onContentClick\nin context?"} Trigger -->|yes| DivBtn["div onClick\n→ calls onContentClick(contentProps)\n(native sheet caller renders StandaloneContent)"] Trigger -->|no| PopoverTrigger["PopoverTrigger\n(opens popover)"] Content --> StandaloneContent["Standalone Content Component\n(BibleChapterPickerContent /\nBibleVersionPickerContent /\nBibleReaderSettingsContent)"] DivBtn -.->|"consumer renders standalone"| StandaloneContentComments Outside Diff (3)
packages/ui/src/components/bible-chapter-picker.tsx, line 422-431 (link)When
onContentClickis provided,BibleChapterPicker.Triggerreplaces<PopoverTrigger>with a plain<div onClick>. A<div>is not in the tab order by default and receives noEnter/Spacekey events, so keyboard-only users cannot open the picker. Switch to<button type="button">or addrole="button",tabIndex={0}, and anonKeyDownhandler to restore keyboard accessibility.Prompt To Fix With AI
packages/ui/src/components/bible-version-picker.tsx, line 1380-1395 (link)BibleVersionPicker.TriggerWhen
onContentClickis set,BibleVersionPicker.Triggeralso renders a<div onClick>wrapper withoutrole,tabIndex, or keyboard handlers. The same<button type="button">fix applies here.Prompt To Fix With AI
packages/ui/src/components/bible-reader.tsx, line 500-505 (link)_currentFontSizereceived but not renderedcurrentFontSizeis accepted inBibleReaderSettingsContentPropsand destructured with an_prefix signalling intentional non-use, yet no current-size indicator is shown in the UI. If the native-sheet consumer passes this to synchronise the displayed size with the reader's state, the silent discard could leave the sheet UI out of sync. Either render the value (e.g., as a label between the two A buttons) or document that it is intentionally unused.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(ui): extract standalone popover con..." | Re-trigger Greptile