Skip to content
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

A few ESLint and Sass-Lint tweaks #171

Merged
merged 2 commits into from Feb 14, 2017
Merged

Conversation

pdehaan
Copy link
Collaborator

@pdehaan pdehaan commented Feb 14, 2017

¯\_(ツ)_/¯

.sass-lint.yml Outdated
no-qualifying-elements: 0
no-vendor-prefixes: 0
property-sort-order: 0
shorthand-values: 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just compulsively A-z sorted these. nbd.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't add or remove anything?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, always better to split these kinds of reorgs / sorts into separate commits, because they can obscure actual changes in diffs

@@ -68,14 +68,14 @@
"clean": "rm -rf dist && shx mkdir -p dist/popup",
"watch": "npm-run-all --parallel watch:*",
"copy:locales": "node pontoon-to-webext",
"copy:assets": "shx cp -r src/manifest.json src/icons dist/ && svgo src/icons -o dist/icons && shx cp -r src/popup/*.html src/popup/FiraSans-Regular.* dist/popup/",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to copy the [unoptimized] src/icons here, since we're copying them in the next step using SVGO during optimization.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct that this wonʼt copy the src/icons/color_bell_icon.png? Should we change that to src/icons/*.png instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not all icons are SVG

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, tweaked this slightly to cp -r src/manifest.json src/icons dist/ && svgo dist/icons --quiet.
That should copy all the "src/icons/*" files (PNG and SVG) into "dist/icons", and then just run svgo optimizer on all the dist/icons/*.svg files and overwrite them in place.

package.json Outdated
@@ -68,14 +68,14 @@
"clean": "rm -rf dist && shx mkdir -p dist/popup",
"watch": "npm-run-all --parallel watch:*",
"copy:locales": "node pontoon-to-webext",
"copy:assets": "shx cp -r src/manifest.json src/icons dist/ && svgo src/icons -o dist/icons && shx cp -r src/popup/*.html src/popup/FiraSans-Regular.* dist/popup/",
"copy:assets": "shx cp -r src/manifest.json dist/ && svgo src/icons -o dist/icons --quiet && shx cp -r src/popup/*.html src/popup/FiraSans-Regular.* dist/popup/",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added --quiet flag, since flooding the console w/ minimization results each time seems unnecessary.

"copy:js": "shx cp node_modules/testpilot-metrics/testpilot-metrics.js dist/",
"bundle:js": "npm run lint:js && rollup -c --environment entry:background && rollup -c --environment entry:popup/snooze-content && rollup -c --environment entry:lib/confirm-bar",
"bundle:css": "npm run lint:sass && shx mkdir -p dist/popup && node-sass src/popup/snooze.scss > dist/popup/snooze.css",
"package": "npm run build && web-ext build -s dist -a .",
"lint": "npm-run-all --parallel lint:*",
"lint:js": "eslint --max-warnings=0 .",
"lint:sass": "sass-lint --max-warnings 0 --verbose --no-exit src/**/*.scss",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the source glob here, since we already define the files to lint in the .sass-lint.yml config file (on line 2).

@@ -20,7 +20,7 @@
"no-unused-vars": "warn",
"no-var": "error",
"prefer-const": "error",
"quotes": ["error", "single"],
"quotes": ["error", "single", {"avoidEscape": true}],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sneakily added this since I was playing w/ prettier on this repo and it was doing some escaping of single quotes in the stories/index.js:

/Users/pdehaan/dev/github/mozilla/SnoozeTabs/stories/index.js
  27:12  error  Strings must use singlequote  quotes
  33:12  error  Strings must use singlequote  quotes
  63:12  error  Strings must use singlequote  quotes
  69:12  error  Strings must use singlequote  quotes

✖ 4 problems (4 errors, 0 warnings)

Copy link
Owner

@bwinton bwinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of questions, but mostly looks good.

.sass-lint.yml Outdated
no-qualifying-elements: 0
no-vendor-prefixes: 0
property-sort-order: 0
shorthand-values: 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't add or remove anything?

@@ -68,14 +68,14 @@
"clean": "rm -rf dist && shx mkdir -p dist/popup",
"watch": "npm-run-all --parallel watch:*",
"copy:locales": "node pontoon-to-webext",
"copy:assets": "shx cp -r src/manifest.json src/icons dist/ && svgo src/icons -o dist/icons && shx cp -r src/popup/*.html src/popup/FiraSans-Regular.* dist/popup/",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct that this wonʼt copy the src/icons/color_bell_icon.png? Should we change that to src/icons/*.png instead?

@pdehaan pdehaan merged commit 093bce8 into bwinton:master Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants