chore/Issue-51: Refactor lint-staged config #112

DevLab2425 · 2024-10-14T14:08:37Z

Related Issue

Fix #51

Summary of Changes

Refactor lint-staged config to run library exec directly

Some Notes on lint-staged:

Running a sequence requires an array - https://www.npmjs.com/package/lint-staged/v/12.3.2#running-multiple-commands-in-a-sequence
Reusing npm scripts seem to require a trailing "--" - https://www.npmjs.com/package/lint-staged/v/12.3.2#reuse-npm-script
When filtering files, using a leading / will change how the glob works to identify the files. - https://www.npmjs.com/package/lint-staged/v/12.3.2#filtering-files
- If the glob pattern contains no slashes (/), micromatch's matchBase option will enabled, so globs match a file's basename regardless of directory
- If the glob pattern does contain a slash (/), it will match for paths as well

Error Report

This is after the stylelint-config-recommended update

src/components/capabilities/capabilities.css
  44:1  ✖  Expected selector ".active strong" to come before selector ".section:hover strong"  no-descending-specificity
  70:1  ✖  Unexpected duplicate selector ".heading", first used at line 12                     no-duplicate-selectors
  88:1  ✖  Unexpected duplicate selector ".sections", first used at line 21                    no-duplicate-selectors

src/components/copy-to-clipboard/copy-to-clipboard.css
  7:3  ✖  Unexpected duplicate "background-color"  declaration-block-no-duplicate-properties

src/components/footer/footer.module.css
  14:3  ✖  Unexpected shorthand "flex" after "flex-basis"  declaration-block-no-shorthand-property-overrides

src/components/header/header.module.css
  111:1  ✖  Expected selector ".mobileMenuListItem a" to come before selector ".navBarMenuItem a:hover"  no-descending-specificity

src/components/hero-banner/hero-banner.module.css
  90:3  ✖  Unexpected duplicate "display"         declaration-block-no-duplicate-properties
  94:3  ✖  Unexpected duplicate "vertical-align"  declaration-block-no-duplicate-properties

src/components/side-nav/side-nav.module.css
  60:3  ✖  Expected selector ".compactMenuSectionHeading a" to come before selector ".compactMenu a:hover"         no-descending-specificity
  65:1  ✖  Unexpected duplicate selector ".compactMenuSectionHeading", first used at line 57                       no-duplicate-selectors
  86:3  ✖  Expected selector ".compactMenuSectionListItemActive a" to come before selector ".compactMenu a:hover"  no-descending-specificity

✖ 11 problems (11 errors, 0 warnings)
  3 errors potentially fixable with the "--fix" option.

From comment #112 (comment)

Swap out stylelint-prettier/recommended (config and dependency) with stylelint-config-recommended in stylelint.config.js -- https://github.com/ProjectEvergreen/www.greenwoodjs.dev/pull/112/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R77
Fix up any new CSS linting checks / sanity test UI
tweak link staged tasks to reflect original intent + best practices -- https://github.com/ProjectEvergreen/www.greenwoodjs.dev/pull/112/files#diff-29c15f0b5d8d1144bd22153c302cde16c2de092d65b122a618cbdb3023336346R2-R4
(bonus / optional / whatever) put lint-staged config in its own config file -- https://github.com/ProjectEvergreen/www.greenwoodjs.dev/blob/71a56b2e8b3a810da0513e9c383746a4edfd341e/lint-staged.config.js

netlify · 2024-10-14T14:09:36Z

✅ Deploy Preview for super-tapioca-5987ce ready!

Name	Link
🔨 Latest commit	`acb5a4e`
🔍 Latest deploy log	https://app.netlify.com/sites/super-tapioca-5987ce/deploys/670d25e80aaa890008ab421f
😎 Deploy Preview	https://deploy-preview-112--super-tapioca-5987ce.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-10-14T14:10:40Z

✅ Deploy Preview for super-tapioca-5987ce ready!

Name	Link
🔨 Latest commit	`e945636`
🔍 Latest deploy log	https://app.netlify.com/sites/super-tapioca-5987ce/deploys/6719a2e283ad86000829e720
😎 Deploy Preview	https://deploy-preview-112--super-tapioca-5987ce.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

thescientist13 · 2024-10-14T16:15:03Z

package.json

-    "*.js": "npm run lint:js",
-    "*.css": "npm run lint:css",
-    "**/*": "npm run lint:ls && npm run format"
+    "*.js": [


Interesting, didn't know about the array syntax, that makes sense. I get the -- part but curious how it works for say this case?

# before "*.css": "npm run lint:css", # after "*.css": "npm run lint:css --",

How come we would need the -- here? Does this imply format is being run implicitly and so needs to be chained in each entry in the lint staged config?

Honestly, I'm not totally clear with why the -- is needed. It was listed in the examples so I assume it's something within lint-staged that requires it.

With npm you use the -- to forward arguments in npm scripts, like here
https://github.com/thescientist13/gallinago/blob/master/package.json#L30

"test": "c8 node --debug-port 3333 ./node_modules/mocha/bin/mocha", "test:tdd": "npm test -- --watch"

I'm wondering if this just enabling some sort of catch-all forwarding here?

thescientist13 · 2024-10-17T02:32:19Z

Let me give this a dry run in a couple days

thescientist13 · 2024-10-17T16:41:14Z

package.json

+      "npm run format --"
+    ],
+    "*.css": "npm run lint:css --",
+    "*.*": "npm run lint:ls --"


just making sure I understand, but this change looks like it would not be running Prettier anymore on all files, just JS files? Is there any way we could keep that previous behavior of formatting all files?

I think this should preserve the existing intentions while also accounting for the best practices you've applied here?

"lint-staged": { "*.js": "npm run lint:js --", "*.css": "npm run lint:css --", "*.*": [ "npm run lint:ls --", "npm run format --" ] }

Related, but I think I may have made a mistake in the original Stylelint configuration, in that it looks like we are still honoring stylistic rules with the plugin I installed (was not my intention), which we shouldn't be doing since that's the whole point with Prettier (seem obvious in hindsight 😅 ).

This also causes tension in our lint staged as it commits the faux pax of having multiple tools trying to enforce / report style rules. (I had setup ESLint to ignore style rules and that looks to be behaving as expected)?

➜ www.greenwoodjs.dev git:(issue-51-resolve-lint-staged) ✗ git commit -m "fix styles" ✔ Preparing lint-staged... ⚠ Running tasks for staged files... ❯ package.json — 2 files ↓ *.js — no files ❯ *.css — 1 file ✖ npm run lint:css -- [FAILED] ❯ *.* — 2 files ✔ npm run lint:ls -- ✖ npm run format -- [KILLED] ↓ Skipped because of errors from tasks. ✔ Reverting to original state because of errors... ✔ Cleaning up temporary files...
✖ npm run lint:css --: src/styles/theme.css 23:7 ✖ Replace "·" with "⏎" prettier/prettier ✖ 1 problem (1 error, 0 warnings) > [email protected] lint:css > stylelint "./src/**/*.css" /Users/owenbuckley/Workspace/project-evergreen/www.greenwoodjs.dev/src/styles/theme.css ✖ npm run format --: > [email protected] format > prettier . --write /Users/owenbuckley/Workspace/project-evergreen/www.greenwoodjs.dev/package.json /Users/owenbuckley/Workspace/project-evergreen/www.greenwoodjs.dev/src/styles/theme.css

I think what I should have done is install stylelint-prettier-recommended which now also benefits from actually catching some lint errors lol 🙃

➜ www.greenwoodjs.dev git:(issue-51-resolve-lint-staged) ✗ npm run lint:css
> [email protected] lint:css > stylelint "./src/**/*.css" src/components/capabilities/capabilities.css 44:1 ✖ Expected selector ".active strong" to come before selector ".section:hover strong" no-descending-specificity 70:1 ✖ Unexpected duplicate selector ".heading", first used at line 12 no-duplicate-selectors 88:1 ✖ Unexpected duplicate selector ".sections", first used at line 21 no-duplicate-selectors src/components/copy-to-clipboard/copy-to-clipboard.css 7:3 ✖ Unexpected duplicate "background-color" declaration-block-no-duplicate-properties src/components/footer/footer.module.css 14:3 ✖ Unexpected shorthand "flex" after "flex-basis" declaration-block-no-shorthand-property-overrides src/components/hero-banner/hero-banner.module.css 90:3 ✖ Unexpected duplicate "display" declaration-block-no-duplicate-properties 94:3 ✖ Unexpected duplicate "vertical-align" declaration-block-no-duplicate-properties src/components/side-nav/side-nav.module.css 69:3 ✖ Expected selector ".compactMenuSectionHeading a" to come before selector ".compactMenu no-descending-specificity a:hover" 74:1 ✖ Unexpected duplicate selector ".compactMenuSectionHeading", first used at line 66 no-duplicate-selectors 95:3 ✖ Expected selector ".compactMenuSectionListItemActive a" to come before selector no-descending-specificity ".compactMenu a:hover" src/components/header/header.module.css 111:1 ✖ Expected selector ".mobileMenuListItem a" to come before selector ".navBarMenuItem no-descending-specificity a:hover" ✖ 11 problems (11 errors, 0 warnings) 3 errors potentially fixable with the "--fix" option.

So with all that said, I think we're pretty close to get this setup properly

Swap out stylelint-prettier/recommended (config and dependency) with stylelint-config-recommended in stylelint.config.js

Fix up any new CSS linting checks / sanity test UI

tweak link staged tasks to reflect original intent + best practices

(bonus / optional / whatever) put lint-staged config in its own config file

DevLab2425 · 2024-10-20T15:37:38Z

I've been able to make a good portion of the lint changes, but struggling with the last 4. These validations that mention the descending-specificity are slightly confusing, and seem to be a bit more risky since the cascading of the styles may not align post-changes.

It seems that even though an element (ie: a given <a>) is selected using one selector, the descending validation also includes different selectors for the same element. I just wanted to bring these to attention for the QA stage.

.parentClass a vs. .mobleClass a. Same anchor being referenced, just a different way to locate it.

src/components/capabilities/capabilities.css
  46:1  ✖  Expected selector ".active strong" to come before selector ".section:hover strong"  no-descending-specificity

src/components/header/header.module.css
  111:1  ✖  Expected selector ".mobileMenuListItem a" to come before selector ".navBarMenuItem a:hover"  no-descending-specificity

src/components/side-nav/side-nav.module.css
  60:3  ✖  Expected selector ".compactMenuSectionHeading a" to come before selector ".compactMenu a:hover"         no-descending-specificity
  84:3  ✖  Expected selector ".compactMenuSectionListItemActive a" to come before selector ".compactMenu a:hover"  no-descending-specificity

✖ 4 problems (4 errors, 0 warnings)

thescientist13 · 2024-10-21T16:22:00Z

Hmm, yeah. It wouldn't be inaccurate to say that some of these styles were written a little hastily at the time. 😅

I would make a best guess attempt at trying to consolidate on the called out styles, likely deferring to the most specific / nested selector element where possible, assuming it still works and looks the same. Otherwise, maybe it could just be a limitation of the tool not knowing our HTML well enough, but more likely just some sloppy CSS.

Otherwise, I am happy to create an issue to specifically capture these specific items on their own so we can at least land this and have proper linting going forward for all net new code.

thescientist13

Sweet! Did a quick test of the site and things are looking good, will do a more thorough check a little later on.

Left a couple feedback items but otherwise I think this one will be good to go on my next review. 👍

thescientist13 · 2024-10-22T13:30:07Z

lint-staged.config.js

@@ -0,0 +1,5 @@
+export default {
+  "*.js": ["npm run lint:js --", "npm run format --"],


Seems like by adding npm run format to both globs now, would it would now be running twice, at least for JS files?

Is it possible to keep format applied only to the wildcard glob?

thescientist13 · 2024-10-22T13:32:38Z

stylelint.config.js

@@ -1,6 +1,3 @@
 export default {
-  // no stylelint 16 support yet
-  // https://github.com/developer-stylechain/stylelint-a11y/issues/4
-  // plugins: ['stylelint-a11y'],


do we know have access to the a11y rules? otherwise, we can should leave this comment in as a reminder, or maybe its just time to make a formal issue for tracking.

Oh, yeah I can revert this, or update to include our own issue, if you prefer.

As for the support, it looks like the issue that you have linked above is (now) closed and has a comment with a resolution of using 2 other libraries instead of the developer-stylechain project.

Styleline v15 -> https://www.npmjs.com/package/@ronilaukkarinen/stylelint-a11y
Styleline v16 -> https://www.npmjs.com/package/@double-great/stylelint-a11y

Did you want me to attempt to include that plugin here in this PR? Happy to try, and could spin off a new PR if things got messy, if that's cool.

thescientist13

Did a run through and looks good! Just made a small tweak to the lint-staged config and a follow up issue for #119

Thanks for another great PR!

thescientist13 reviewed Oct 14, 2024

View reviewed changes

DevLab2425 marked this pull request as ready for review October 14, 2024 16:22

DevLab2425 added bug Something isn't working hacktoberfest-accepted Used to participate in Hacktoberfest labels Oct 14, 2024

thescientist13 added chore project maintenance related tasks and removed bug Something isn't working labels Oct 15, 2024

thescientist13 self-assigned this Oct 17, 2024

thescientist13 reviewed Oct 17, 2024

View reviewed changes

DevLab2425 added 6 commits October 21, 2024 18:49

Issue-51: Refactor lint-staged config

96a3e72

Issue-51: Refactor to reuse npm scripts

2cabff7

Issue-51: Initial commit of lint-stage.config

7b8f032

Replace stylelint plugin

248262a

Issue-51: Update invalid styles after lint report

6cd4804

Issue-51: Resolve stylelint errors

71a56b2

DevLab2425 force-pushed the issue-51-resolve-lint-staged branch from 8be3061 to 71a56b2 Compare October 21, 2024 22:49

Issue-51: Clean up

fe811a3

thescientist13 reviewed Oct 22, 2024

View reviewed changes

tweak JS glob lint staged rule

e945636

thescientist13 changed the title ~~Issue-51: Refactor lint-staged config~~ chore/Issue-51: Refactor lint-staged config Oct 24, 2024

thescientist13 mentioned this pull request Oct 24, 2024

setup project with Stylelint a11y plugin #119

Closed

thescientist13 approved these changes Oct 24, 2024

View reviewed changes

thescientist13 merged commit be1d0da into main Oct 24, 2024
5 checks passed

thescientist13 deleted the issue-51-resolve-lint-staged branch October 24, 2024 01:32

thescientist13 mentioned this pull request Oct 28, 2024

chore/Issue-119: Setup project with Stylelint a11y plugin #121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore/Issue-51: Refactor lint-staged config #112

chore/Issue-51: Refactor lint-staged config #112

DevLab2425 commented Oct 14, 2024 •

edited

Loading

netlify bot commented Oct 14, 2024

netlify bot commented Oct 14, 2024 •

edited

Loading

thescientist13 Oct 14, 2024 •

edited

Loading

DevLab2425 Oct 14, 2024

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 commented Oct 17, 2024

thescientist13 Oct 17, 2024 •

edited

Loading

DevLab2425 commented Oct 20, 2024

thescientist13 commented Oct 21, 2024 •

edited

Loading

thescientist13 left a comment

thescientist13 Oct 22, 2024

thescientist13 Oct 22, 2024

DevLab2425 Oct 23, 2024

thescientist13 left a comment

		@@ -0,0 +1,5 @@
		export default {
		"*.js": ["npm run lint:js --", "npm run format --"],

chore/Issue-51: Refactor lint-staged config #112

chore/Issue-51: Refactor lint-staged config #112

Conversation

DevLab2425 commented Oct 14, 2024 • edited Loading

Related Issue

Summary of Changes

Error Report

netlify bot commented Oct 14, 2024

✅ Deploy Preview for super-tapioca-5987ce ready!

netlify bot commented Oct 14, 2024 • edited Loading

✅ Deploy Preview for super-tapioca-5987ce ready!

thescientist13 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

DevLab2425 Oct 14, 2024

Choose a reason for hiding this comment

thescientist13 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

thescientist13 commented Oct 17, 2024

thescientist13 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

DevLab2425 commented Oct 20, 2024

thescientist13 commented Oct 21, 2024 • edited Loading

thescientist13 left a comment

Choose a reason for hiding this comment

thescientist13 Oct 22, 2024

Choose a reason for hiding this comment

thescientist13 Oct 22, 2024

Choose a reason for hiding this comment

DevLab2425 Oct 23, 2024

Choose a reason for hiding this comment

thescientist13 left a comment

Choose a reason for hiding this comment

DevLab2425 commented Oct 14, 2024 •

edited

Loading

netlify bot commented Oct 14, 2024 •

edited

Loading

thescientist13 Oct 14, 2024 •

edited

Loading

thescientist13 Oct 15, 2024 •

edited

Loading

thescientist13 Oct 17, 2024 •

edited

Loading

thescientist13 commented Oct 21, 2024 •

edited

Loading