-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[No QA] Create a convenience script to quickly lint changed files locally #18342
Conversation
npm has a |
@thesahindia @tylerkaraszewski One of you needs to 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] |
@thesahindia I don't think we really need a C+ review here. |
npm has a |
1 similar comment
npm has a |
Conflicts resolved |
Do we really need to do the whole checklist for this? |
I'd say fill out the checklist so tests pass but no need for screenshots on all platforms |
I'm going to merge this w/o the reviewer checklist since there's no screenshots or product changes involved and it's kind of a useful little script. |
@roryabraham looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Not an emergency, see the above ^ |
✋ 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: 1.3.26-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.26-4 🚀
|
Details
Running lint on all files can take about 5 minutes and some people have even reported it failing due to running out of memory. This is just a minor QOL / productivity improvement – run
npm run lint-changed
to quickly lint + fix changed files between your current branch andmain
. Has no effect if used on themain
branch.Fixed Issues
$ #18339
Tests
npm i
, if necessarynpm run lint-changed
. Verify that the lint errors are shown and that correctable errors (such as missing trailing comma) are automatically fixed.Offline tests
None
QA Steps
None
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
Not platform specific, but here you go:
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android