-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
chore: standardize npm script names #17431
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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. Waiting for code owner's review.
docs/package.json
Outdated
"lint:fix:scss": "npm run lint:scss -- --fix" | ||
"lint:scss": "stylelint \"**/*.{scss,html}\"", | ||
"lint:fix:scss": "npm run lint:scss -- --fix", | ||
"minify:images": "imagemin '_site/assets/images' --out-dir='_site/assets/images'", |
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.
Maybe this should also be build
?
"minify:images": "imagemin '_site/assets/images' --out-dir='_site/assets/images'", | |
"build:images": "imagemin '_site/assets/images' --out-dir='_site/assets/images'", |
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.
I feel like we could have it as minify:images
only? or it could be build:images:minify
?
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.
We can update it to build:minify-images
,WDYT?
Just as a reminder, we do have documentation about naming conventions for these scripts: |
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, thanks! Leaving open for others to review.
I think we don't need to update anything on Netlify/Jenkins. Netlify only runs |
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. leaving it open for @nzakas's review
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. Thanks!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
What changes did you make? (Give an overview)
Refers #14827
This updates the names of the scripts in package.json to be consistent with the new standard.
Is there anything you'd like reviewers to focus on?
Is there any script we need to update in Netlify/jenkins?