-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Bring "[HybridApp] Necessary copilot changes" back #55392
Bring "[HybridApp] Necessary copilot changes" back #55392
Conversation
@DylanDylann 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] |
cc @chiragsalian @dangrous This is the new version of the reverted copilot PR |
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! 🎉
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/chiragsalian in version: 9.0.88-0 🚀
|
Hi @izarutskaya 👋 We have one (almost merged) PR on OD side which is required for this feature to work correctly. Once it's merged the issue should be resolved |
Looks like the OD Pr just got merged, which is great! So we're good to check this off the deploy checklist, right? :D |
@Beamanator I think so, but it'd be good to check if it's working now |
Ooh good question - I believe we "used to" need to CP anything in the App repo in order to pull changes into Hybrid App staging builds, but i thinkkk we're "in the process" of updating so we can CP hybrid app changes directly - any idea what's the status on that @Julesssss ? |
There's no staging for Mobile-Expensify and we currently update the submodule commit on each new build. We do plan to change this so that the checklist |
Ok well I am planning a few CP's very soon, so that should pull in the ME changes so we can test it all out before the next deploy (hopefully soon) 👍 |
Huh, so I was trying to test this out to see if it's working, but what I saw was: In NewDot when I was copiloted, I wasn't allowed to switch to classic: Screen.Recording.2025-01-23.at.11.11.04.AM.movHowever, when I switched BACK to my normal account, I was able to freely switch between classic & newdot with no troubles 👍 Is that the expected result? I don't see ANY testing videos from anyone in this PR which is... interesting 😅 |
Seems to be workin well on latest HybridApp! |
🚀 Deployed to production by https://github.com/puneetlath in version: 9.0.88-7 🚀
|
Explanation of Change
Related OD PR: https://github.com/Expensify/Mobile-Expensify/pull/13289
Fixed Issues
$ #51264
PROPOSAL:
Tests
Try New Experience
orSwitch to Expensify Classic
)Offline tests
QA Steps
Try New Experience
orSwitch to Expensify Classic
)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop