-
Notifications
You must be signed in to change notification settings - Fork 0
[MOB-11120] add-iterable-to-test-app #4
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
Conversation
… peer dependency for Iterable SDK
…elated dependencies
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.
Pull Request Overview
This PR integrates Iterable into the test app and adds relevant color constants for styling consistency.
- Adds Iterable dependency to iOS podspec and integrates Iterable SDK in the login flow.
- Refactors module imports and updates color constants to support Iterable features.
- Updates file structure by moving App.tsx into the src folder and removing the old implementation.
Reviewed Changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ios/ExpoAdapterIterable.podspec | Adds Iterable dependency for React Native integration. |
| example/src/index.ts | Exports the App component from the updated location. |
| example/src/constants/index.ts | Re-exports color constants from colors.ts. |
| example/src/constants/colors.ts | Defines extensive color constants and types for styling. |
| example/src/Login.tsx | Implements the login screen with Iterable initialization. |
| example/src/App.tsx | Configures navigation between login and Iterable inbox. |
| example/index.ts | Updates the entry point to use the new src folder. |
| example/App.tsx | Removes outdated App implementation. |
| .github/pull_request_template.md | Adds pull request template with instructions and screenshots. |
Files not reviewed (4)
- android/src/main/java/expo/modules/adapters/iterable/ExpoAdapterIterableModule.kt: Language not supported
- example/package.json: Language not supported
- example/tsconfig.json: Language not supported
- package.json: Language not supported
…iterable-to-test-app
…ors type reference
…om/Iterable/iterable-expo-plugin into MOB-11120-add-iterable-to-test-app * 'MOB-11120-add-iterable-to-test-app' of https://github.com/Iterable/iterable-expo-plugin:
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.
Pull Request Overview
This PR integrates Iterable into the test app, enabling an in-app Iterable inbox feature. Key changes include adding an Iterable dependency in the iOS podspec, refactoring the file structure to support the new integration, and introducing a new Login component that initializes and sets up Iterable.
Reviewed Changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ios/ExpoAdapterIterable.podspec | Added dependency on Iterable-React-Native-SDK |
| example/src/index.ts | Updated export path to load App from the appropriate source directory |
| example/src/constants/colors.ts | Added comprehensive color constants supporting consistent app theming |
| example/src/Login.tsx | Introduced a Login component that initializes Iterable and triggers login flow |
| example/src/App.tsx | Updated App component to conditionally render Iterable inbox or Login |
| example/index.ts | Modified import path to adapt to the new file structure |
| example/App.tsx (removed) | Removed obsolete App file |
| .github/pull_request_template.md | Added pull request template for consistent PR documentation |
Files not reviewed (4)
- android/src/main/java/expo/modules/adapters/iterable/ExpoAdapterIterableModule.kt: Language not supported
- example/package.json: Language not supported
- example/tsconfig.json: Language not supported
- package.json: Language not supported
Comments suppressed due to low confidence (1)
example/index.ts:3
- Ensure that the updated import path correctly references the intended module, as the change from './App' to './src' may lead to naming mismatches if the exported component is not properly aligned.
import App from './src';
| const onPress = () => { | ||
| Iterable.setEmail(email); | ||
| setTimeout(() => { | ||
| onLoggedIn(); | ||
| }, 300); |
Copilot
AI
Apr 1, 2025
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.
Using a fixed delay with setTimeout for navigation may cause timing issues on slower devices. Consider using a callback or promise resolution from the Iterable initialization to ensure reliable transition.
| const onPress = () => { | |
| Iterable.setEmail(email); | |
| setTimeout(() => { | |
| onLoggedIn(); | |
| }, 300); | |
| const onPress = async () => { | |
| Iterable.setEmail(email); | |
| await Iterable.initialize(getApiKey(), new IterableConfig()); | |
| onLoggedIn(); |
…e app dependencies
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.
Pull Request Overview
This PR adds Iterable integration to the test app by updating dependencies, reorganizing the project structure, and implementing a login screen that initializes the Iterable SDK.
- Updated the iOS podspec to include the Iterable React Native SDK.
- Introduced new files in the example app for colors, constants, and login functionality.
- Adjusted the app's entry point and removed a dummy App component to streamline the integration.
Reviewed Changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ios/ExpoAdapterIterable.podspec | Added dependency for Iterable React Native SDK |
| example/src/index.ts | Updated export to point to the App component |
| example/src/constants/{index,colors}.ts | Introduced and organized colors and constants |
| example/src/Login.tsx | Added Login component for Iterable initialization |
| example/src/App.tsx | Integrated IterableInbox with conditional rendering |
| example/index.ts | Updated file import to reflect new project structure |
| example/App.tsx | Removed outdated App component |
| .github/pull_request_template.md | Added PR template for documentation and testing guidance |
Files not reviewed (4)
- example/android/app/src/main/AndroidManifest.xml: Language not supported
- example/package.json: Language not supported
- example/tsconfig.json: Language not supported
- package.json: Language not supported
Comments suppressed due to low confidence (2)
example/src/Login.tsx:80
- [nitpick] Consider using onPress instead of onPressOut to ensure a more standard and consistent user interaction for triggering the login action.
onPressOut={onPress}
example/src/Login.tsx:54
- [nitpick] Consider replacing the hardcoded 300ms delay with a named constant to improve readability and maintainability.
setTimeout(() => {
evantk91
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.
LGTM. Verified with iOS and android. Just some comments.
| Please explain the steps needed to verify your change. | ||
|
|
||
| ## 📝 Documentation | ||
|
|
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.
Not sure what this file is used for. Can we remove?
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.
It's supposed to give a base template for the pull requests, but it's not working for some reason
| </queries> | ||
| <application android:name=".MainApplication" android:label="@string/app_name" android:icon="@mipmap/ic_launcher" android:roundIcon="@mipmap/ic_launcher_round" android:allowBackup="true" android:theme="@style/AppTheme" android:supportsRtl="true"> | ||
| <meta-data android:name="ITERABLE_API_KEY" android:value="YOUR_ITERABLE_API_KEY"/> | ||
| <meta-data android:name="ITERABLE_API_KEY" android:value="be63942e7f1e425b9923c4d86596528c"/> |
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.
Looks like API key was accidently pushed up
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.
shoot!!!! i'll disable it
| "main": "index.ts", | ||
| "scripts": { | ||
| "start": "expo start", | ||
| "start": "expo start --dev-client", |
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.
What does --dev-client do?
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.
makes sure that it doesn't run with expo go, which cannot work with our app since it requires that you do not use native files (ie: swift, kotlin, etc)
Add iterable integration
Test
npm installnpm run preparenpm installnpx expo prebuild --clean-- android:
npx expo run:android-- ios:
npx expo run: iosScreenshots