Conversation
Greptile SummaryThis PR brings the Android Compose views up to parity with the Swift SDK —
Confidence Score: 4/5Safe to merge with minor follow-up needed; VotdView JS API has functional gaps Two P1 issues: share/full-chapter events and compact mode are unreachable from JS due to missing TypeScript props. Most prior concerns are resolved (VerseTappedEvent, @RequiresApi, versionId fallback, onTap for BibleText). Score 4 because the VotdView JS API gap is a real functional defect preventing users from subscribing to callbacks or using the compact variant. src/components/VotdView.tsx — missing onSharePress, onFullChapterPress, isCompact, showIcon props Important Files Changed
Sequence DiagramsequenceDiagram
participant JS as JS (React Native)
participant TS as VotdView.tsx
participant RN as RNVotdViewModule
participant Compose as YVPVotdView
participant SDK as VerseOfTheDay SDK
JS->>TS: render VotdView bibleVersionId=3034
TS->>RN: NativeView (bibleVersionId, colorScheme only)
RN->>Compose: YVPVotdView(props, onSharePress, onFullChapterPress)
Compose->>SDK: VerseOfTheDay(onShareClick, onFullChapterClick)
SDK-->>Compose: user taps Share button
Compose-->>RN: onSharePress() triggers EventDispatcher
RN--xJS: event dropped — onSharePress not in VotdViewProps/NativeProps
Note over JS,TS: isCompact and showIcon also unreachable from JS
Reviews (6): Last reviewed commit: "chore: make urlScheme and footnotes opti..." | Re-trigger Greptile |
android/src/main/java/com/youversion/reactnativesdk/views/YVPBibleTextView.kt
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/views/YVPSignInWithYouVersionButton.kt
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/views/YVPBibleTextView.kt
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/views/YVPBibleTextView.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/views/YVPBibleWidgetView.kt
Show resolved
Hide resolved
camrun91
left a comment
There was a problem hiding this comment.
A few changes and the greptile issues look legit too.
android/src/main/java/com/youversion/reactnativesdk/views/YVPVotdView.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/views/YVPVotdView.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/views/YVPVotdView.kt
Show resolved
Hide resolved
This isn't a change in functionality, but it matches what Expo is doing with their Compose components.
It looks like Expo no longer supports piping props into mutable state. We should use the regular primitives instead.
96ac894 to
74d74e3
Compare
bmanquen
left a comment
There was a problem hiding this comment.
Thank you for all of this. Nothing blocking, just a nitpick, a possible abstraction that can be made.
I only saw it in 2 or 3 places, so I am fine either way.
| return BibleTextOptions(fontSize = props.fontSize.sp) | ||
| } | ||
|
|
||
| fun bibleReference(props: BibleWidgetViewProps): BibleReference { |
There was a problem hiding this comment.
Should these be like a utility function abstracted away from these view files? It seems like you have used the same function multiple times.
Description
This brings the Android side of the RN SDK up to date with the Swift SDK. There are still a few UI bugs that occur when the components load. However, those can be tackled in future work.
Type of Change
feat:New feature (non-breaking change which adds functionality)fix:Bug fix (non-breaking change which fixes an issue)docs:Documentation updaterefactor:Code refactoring (no functional changes)perf:Performance improvementtest:Test additions or updatesbuild:Build system or dependency changesci:CI configuration changeschore:Other changes (maintenance, etc.)Checklist