-
Notifications
You must be signed in to change notification settings - Fork 1
YPE-1168 - Fix react native example app builds #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Greptile OverviewGreptile SummaryUpdated React Native SDK to support YouVersion Platform SDK 0.6.0, fixing build issues in the example app. Key Changes
Issues Found
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant RN as React Native App
participant Module as RN Platform Module
participant API as YVP API (Kotlin/Swift)
participant SDK as YouVersion SDK 0.6.0
Note over RN,SDK: Bible Versions API Flow (Android)
RN->>Module: getBibleVersions(languageTag)
Module->>API: YVPBibleApi.versions(languageTag)
API->>SDK: YouVersionApi.bible.versions(languageTag)
SDK-->>API: PaginatedResponse<BibleVersion>
Note over API: Cannot import PaginatedResponse type
API->>API: response.extractPaginatedData()
Note over API: Uses reflection to access 'data' field
API->>API: dataList.map { BibleVersionRecord(it) }
API-->>Module: List<BibleVersionRecord>
Module-->>RN: Bible versions array
Note over RN,SDK: User Info API Flow (iOS)
RN->>Module: getUserInfo()
Note over Module: No accessToken param needed
Module->>API: YVPAuthAPI.userInfo(promise)
API->>SDK: YouVersionAPI.Users.currentUser*
Note over SDK: Uses SDK's stored current user
SDK-->>API: User properties
API-->>Module: User info object
Module-->>RN: User info
Note over RN,SDK: Sign In Button (Android)
RN->>Module: Render SignInButton
Module->>API: YVPSignInWithYouVersionButton.Content()
Note over API: Wraps button in clickable Box
API->>SDK: SignInWithYouVersionButton(permissions)
Note over SDK: Requires permissions parameter in 0.6.0
SDK-->>API: Rendered button
API-->>RN: Display button
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 3 comments
android/src/main/java/com/youversion/reactnativesdk/RNYouVersionPlatformModule.kt
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/RNYouVersionPlatformModule.kt
Outdated
Show resolved
Hide resolved
|
@greptileai This code has been updated can you review it again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 3 comments
android/src/main/java/com/youversion/reactnativesdk/api/YVPBibleApi.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/api/YVPLanguagesApi.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/api/YVPRecords.kt
Outdated
Show resolved
Hide resolved
|
@greptileai latest Push has some clean up and should be reviewed again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, 3 comments
android/src/main/java/com/youversion/reactnativesdk/api/PaginatedResponseExtensions.kt
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/api/YVPRecords.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/youversion/reactnativesdk/views/YVPSignInWithYouVersionButton.kt
Show resolved
Hide resolved
|
@mic-mart @sidorchukandrew this builds for me and and this PR should be the minimum of what is needed for example apps to build. |
bmanquen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to see what @sidorchukandrew says about all of this as I am trying to learn more about Android and iOS.
android/src/main/java/com/youversion/reactnativesdk/api/PaginatedResponseExtensions.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that we want to commit or is this just documented reasoning/information for us to read right now?
Description
Update the example code and needed configs to successfully build and run.
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