-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD] Add expo-modules to application. #28588
Conversation
@stitesExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@mczernek lint and jstests are failing |
Reviewer Checklist
Screenshots/VideosAndroid |
I think this PR is causing the following error on iOS Native: @mczernek Can you please double check? |
@stitesExpensify Can you please add the build label so we can test if the builds are working fine? |
Kicked off a test build: https://github.com/Expensify/App/actions/runs/6388210867 |
@allroundexperts I don't think this is caused by this PR. Would you mind removing caches? Remove |
Can we also rerun the builds? I hope forgotten patch should fix android |
Hi @roryabraham @stitesExpensify, Can you please trigger the build again? |
Can we rerun android build? I am not able to reproduce this failure and wanted to make sure it is persistent. |
Created a new build https://github.com/Expensify/App/actions/runs/6407529055 |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪
|
I am placing a HOLD on it, let's discuss in this Slack thread. |
I think we can close this and add expo-modules along with the first library that actually needs it, most probably expo-av. We don't need to add this dependency separately without any actual usage, because that's just unnecessary bloat. |
Details
We are planning on adding some Expo modules to the app in the near future. Since there are several places that are about to work on that in parallel, common part of adding expo-modules infrastructure is done in this PR so that others can benefit from that.
cc @roryabraham
Added bonus is that we can spend some time with just this boilerplate and enforced changes to see whether they break anything.
Fixed Issues
There are not issues associated with this PR.
$
PROPOSAL:
Original Problem/Solution was posted here: https://expensify.slack.com/archives/C01GTK53T8Q/p1695892593681869
Tests
There are no specific tests added.
QA Steps
App should look and feel the same as before this addition and no building flow should be affected for any platform.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web_desktop_chrome.mov
Mobile Web - Chrome
android_web.mp4
Mobile Web - Safari
ios_web.mov
Desktop
desktop.mov
iOS
ios_native.mov
Android
android_web.mp4