-
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
Fixed prefix script commands with ./scripts #48884
Fixed prefix script commands with ./scripts #48884
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
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.
one question and one minor edit. Otherwise seems fine! Ran a couple scripts and they still work for me on Mac. Defer to @roryabraham for final say, though.
I have read the CLA Document and I hereby sign the CLA |
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! All yours, @roryabraham (and please fill out the checklist when you can @1subodhpathak )
Verify signed commits / verifySignedCommits- this is failing, so what do I need to do? Shall I recommit after adding GPG keys? |
yes, you have to set up commit signing and rebase |
getting some errors while rebase, quickly solving it |
2aa99e4
to
eb39dc8
Compare
eb39dc8
to
39e3d5c
Compare
@roryabraham all signed, please verify |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
Reassure Performance Tests is not checked, so this needs to be fixed at my end or what... @roryabraham |
nope, that failure appears unrelated in this case. Thanks for the contribution. |
✋ 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/roryabraham in version: 9.0.36-1 🚀
|
@roryabraham @dangrous Could you help with QA steps? |
no qa needed here @kavimuru, you're all set |
🚀 Deployed to production by https://github.com/grgia in version: 9.0.36-2 🚀
|
@roryabraham
Details
This PR addresses an issue where the
npm install
command fails on Windows machines using WSL due to the script path not being recognized correctly. The changes involve prefixing script commands with ./ to ensure compatibility across both macOS and Linux-based systems (including WSL).Fixed Issues
$
PROPOSAL:
Tests
npm install
in the project directorynpm install
and verify it still works as expectedOffline tests
QA Steps
npm install
and verify it completes successfully without any "not found" errors for scriptsPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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