Add Stylelint #3736

julieg18 · 2023-04-21T16:21:09Z

adds stylelint, a css linter, to the project

Demo

Screen.Recording.2023-04-21.at.3.37.25.PM.mov

package.json

julieg18 · 2023-04-21T19:57:32Z

.stylelintrc.js

@@ -0,0 +1,10 @@
+module.exports = {


Technically, only webview has CSS but I chose to make it global since the VSCode extension for stylelint requires the workspace to have stylelint and config file to be in the root of the workspace. Any reasons not too though?

Can we move as set stylelint.config in the .vscode/settings file? or is there another option?

And do we need to disable the built-in linters as per: https://marketplace.visualstudio.com/items?itemName=stylelint.vscode-stylelint#disable-vs-codes-built-in-linters-optional?

I got the hot tip that:

I’d start by extending stylelint-config-sass-guidelines and stylelint-config-prettier

And do we need to disable the built-in linters as per: https://marketplace.visualstudio.com/items?itemName=stylelint.vscode-stylelint#disable-vs-codes-built-in-linters-optional?

Good idea! Disabled now.

I’d start by extending stylelint-config-sass-guidelines and stylelint-config-prettier

On further research stylelint-config-prettier is depreciated and sass-guidelines is indeed in use inside of our current scss plugin!

Can we move as set stylelint.config in the .vscode/settings file? or is there another option?

I managed to move our stylelint config file to be inside of webview but I couldn't figure out a way to move stylelint packages inside of webview.

I tried to install stylelint packages inside of webview via the stylelint.stylelintPath config option, but stylelint kept getting installed inside the root for some reason, even after moving packages inside of webview/package.json, deleting node_modules, and starting out of a fresh install. I even tried to revert yarn.lock to a previous commit and fresh install the packages but stylelint still appeared inside of /node_modules instead of /webview/node_modules. Kept the packages installed in the root for now but can revisit this later

julieg18 · 2023-04-21T20:04:00Z

webview/src/plots/components/ribbon/styles.module.scss

-    &:hover .copyButton {
-      opacity: 1;
-    }
+.tooltipColumn {


Almost all changes in the pr were done with the --fix flag and I made three manual changes:

no-descending-specificity issue in /ribbon

no-descending-specificity issue in /plots

at-extend-no-missing-placeholder in /table

julieg18 · 2023-04-21T20:09:41Z

webview/src/experiments/components/table/styles.module.scss

@@ -1,11 +1,11 @@
+/* stylelint-disable no-descending-specificity */


table/styles.module.scss had a lot of errors with no-descending-specificity. I disabled this file rule for now since it will take some time to take care of.

dvc-vscode-webview:lint:css: src/experiments/components/table/styles.module.scss dvc-vscode-webview:lint:css: 222:1 ✖ Expected selector ".experimentsTh" to come before selector ".experimentsTh:first-child" no-descending-specificity dvc-vscode-webview:lint:css: 284:3 ✖ Expected selector ".firstLevelHeaderCell .paramHeaderCell" to come before selector ".headRow:last-child .paramHeaderCell" no-descending-specificity dvc-vscode-webview:lint:css: 289:3 ✖ Expected selector ".firstLevelHeaderCell .metricHeaderCell" to come before selector ".headRow:last-child .metricHeaderCell" no-descending-specificity dvc-vscode-webview:lint:css: 294:3 ✖ Expected selector ".firstLevelHeaderCell .depHeaderCell" to come before selector ".headRow:last-child .depHeaderCell" no-descending-specificity dvc-vscode-webview:lint:css: 330:3 ✖ Expected selector ".iconMenu ul[role='menu']" to come before selector ".dropTargetHeaderCell .iconMenu ul[role='menu']" no-descending-specificity dvc-vscode-webview:lint:css: 352:3 ✖ Expected selector ".moveToRight ul[role='menu']" to come before selector ".dropTargetHeaderCell .iconMenu ul[role='menu']" no-descending-specificity dvc-vscode-webview:lint:css: 507:1 ✖ Expected selector ".experimentsTd" to come before selector ".experimentsTd:first-child" no-descending-specificity dvc-vscode-webview:lint:css: 514:3 ✖ Expected selector ".experimentsTd:not(.experimentCell)::before" to come before selector ".bodyRow:hover .experimentsTd:not(.experimentCell):hover::before" no-descending-specificity dvc-vscode-webview:lint:css: 514:3 ✖ Expected selector ".experimentsTd:not(.experimentCell)::before" to come before selector ".bodyRow:hover .experimentsTd:hover + .experimentsTd::before" no-descending-specificity dvc-vscode-webview:lint:css: 514:3 ✖ Expected selector ".experimentsTd:not(.experimentCell)::before" to come before selector ".rowSelected:hover .experimentsTd:not(.experimentCell):hover::before" no-descending-specificity dvc-vscode-webview:lint:css: 514:3 ✖ Expected selector ".experimentsTd:not(.experimentCell)::before" to come before selector ".rowSelected:hover .experimentsTd:hover + .experimentsTd::before" no-descending-specificity dvc-vscode-webview:lint:css: 536:3 ✖ Expected selector ".experimentsTd:first-child" to come before selector ".rowSelected .experimentsTd:not(.experimentCell)" no-descending-specificity dvc-vscode-webview:lint:css: 536:3 ✖ Expected selector ".experimentsTd:first-child" to come before selector ".bodyRow:not(.rowSelected):hover .experimentsTd" no-descending-specificity dvc-vscode-webview:lint:css: 536:3 ✖ Expected selector ".experimentsTd:first-child" to come before selector ".bodyRow:not(.rowSelected):hover .experimentsTd:not(.experimentCell)" no-descending-specificity dvc-vscode-webview:lint:css: 542:3 ✖ Expected selector ".experimentsTd:last-child" to come before selector ".rowSelected .experimentsTd:not(.experimentCell)" no-descending-specificity dvc-vscode-webview:lint:css: 542:3 ✖ Expected selector ".experimentsTd:last-child" to come before selector ".bodyRow:not(.rowSelected):hover .experimentsTd" no-descending-specificity dvc-vscode-webview:lint:css: 542:3 ✖ Expected selector ".experimentsTd:last-child" to come before selector ".bodyRow:not(.rowSelected):hover .experimentsTd:not(.experimentCell)" no-descending-specificity dvc-vscode-webview:lint:css: 549:1 ✖ Expected selector ".innerCell" to come before selector ".experimentsTd:first-child .innerCell" no-descending-specificity dvc-vscode-webview:lint:css: 549:1 ✖ Expected selector ".innerCell" to come before selector ".experimentsTd:last-child .innerCell" no-descending-specificity dvc-vscode-webview:lint:css: 567:3 ✖ Expected selector ".experimentCell .innerCell" to come before selector ".experimentsTd:first-child .innerCell" no-descending-specificity dvc-vscode-webview:lint:css: 567:3 ✖ Expected selector ".experimentCell .innerCell" to come before selector ".experimentsTd:last-child .innerCell" no-descending-specificity dvc-vscode-webview:lint:css: 578:1 ✖ Expected selector ".rowActions" to come before selector ".workspaceRowGroup .rowActions" no-descending-specificity dvc-vscode-webview:lint:css: 619:3 ✖ Expected selector ".starSwitch svg" to come before selector ".iconMenu ul[role='menu'] button svg" no-descending-specificity dvc-vscode-webview:lint:css: 622:5 ✖ Expected selector ".rowSelected .starSwitch svg" to come before selector ".iconMenu ul[role='menu'] button svg" no-descending-specificity dvc-vscode-webview:lint:css: 627:3 ✖ Expected selector ".starSwitch[aria-checked='true'] svg" to come before selector ".iconMenu ul[role='menu'] button svg" no-descending-specificity dvc-vscode-webview:lint:css: 748:3 ✖ Expected selector ".experimentCellText > *" to come before selector ".experimentsTable.withExpColumnShadow .experimentsTr > *:first-child" no-descending-specificity dvc-vscode-webview:lint:css: 759:3 ✖ Expected selector ".experimentCellSecondaryName > *" to come before selector ".experimentsTable.withExpColumnShadow .experimentsTr > *:first-child" no-descending-specificity dvc-vscode-webview:lint:css: 763:3 ✖ Expected selector ".experimentCellSecondaryName svg" to come before selector ".iconMenu ul[role='menu'] button svg" no-descending-specificity dvc-vscode-webview:lint:css: 763:3 ✖ Expected selector ".experimentCellSecondaryName svg" to come before selector ".rowSelected .starSwitch svg" no-descending-specificity dvc-vscode-webview:lint:css: 763:3 ✖ Expected selector ".experimentCellSecondaryName svg" to come before selector ".starSwitch[aria-checked='true'] svg" no-descending-specificity dvc-vscode-webview:lint:css: 784:3 ✖ Expected selector ".experimentCellSecondaryNameTooltip *:first-child" to come before selector ".experimentsTable.withExpColumnShadow .experimentsTr > *:first-child" no-descending-specificity dvc-vscode-webview:lint:css: 794:5 ✖ Expected selector ".experimentCellSecondaryNameTooltip .sha svg" to come before selector ".iconMenu ul[role='menu'] button svg" no-descending-specificity dvc-vscode-webview:lint:css: 814:1 ✖ Expected selector ".timestampInnerCell" to come before selector ".workspaceRowGroup .timestampInnerCell" no-descending-specificity dvc-vscode-webview:lint:css: 867:3 ✖ Expected selector ".commitsAndBranchesNavButton:disabled" to come before selector ".commitsAndBranchesNavButton:hover:not(:disabled)" no-descending-specificity dvc-vscode-webview:lint:css: dvc-vscode-webview:lint:css: 34 problems (34 errors, 0 warnings)

Personally, I think taking care of these will lower the chance of specificity issues and clean things up more but we could also consider turning off this rule entirely if we think it's too strict to upkeep.

If it is a useful rule we should fix.

I think it would be really good to fix these. It might be too big for this PR, but a follow up just for this would be great.

Glad we're all in agreement! Will plan to do this in a followup.

julieg18 · 2023-04-21T20:13:33Z

.stylelintrc.js

@@ -0,0 +1,10 @@
+module.exports = {
+  extends: 'stylelint-config-standard-scss',


Here's the config for stylelint-config-standard-scss. There are also other rules available plus plugins that we could consider enabling.

[Q] Any rules that are missing that we should have turned on?

I think some of the rules for "Max and min" would be useful for us as most of our current problems originate from that.

max-nesting-depth
selector-max-attribute
selector-max-class
selector-max-combinator
selector-max-type <- wondering if setting this to 0 would work
selector-max-specificity

Added max and min rules!

'max-nesting-depth': 2, 'selector-max-attribute': 2, 'selector-max-class': 2, 'selector-max-combinators': 3, 'selector-max-type': 2,

Had multiple accounts of selector-max-class in table so added a disable comment and plan to fix them in a followup!

I think having a max of two for each of these is a good thing to aim for in the future but we could discuss numbers for each in the future!

'selector-max-type': 2, it's fine for now, but since we just changed some type selectors to classes, I feel like 1 would be more appropriate (if 0 does not work).

julieg18 · 2023-04-21T20:16:12Z

.stylelintrc.js

+module.exports = {
+  extends: 'stylelint-config-standard-scss',
+  rules: {
+    'custom-property-pattern': null,


We have a mixture of camelCase and kebab case rules in our styles thanks to our own CSS module classes, targeting 3rd party components like tippy, and VSCode variables using a mixture of both. I disabled pattern rules, but we could also set things for camelCase and disable certain files.

we can't control other packages but I think we use camelCase, right? We should turn on and exclude files/lines where we can't update. It will probably encourage us to wrap external modules in specific places instead of using them all throughout the code.

All kebab-case rules should be inside :global(...). Looking at stylelint configure docs, I've stumbled upon this:

{ "rules": { "selector-pseudo-class-no-unknown": [ true, { "ignorePseudoClasses": ["global"] } ] } }

Not sure if it applies in this case, but if we can ignore global we could set the rule correctly to camelCase.

On another note (out of scope), unless it's really specific to the current CSS module (.myModuleBlock :global(.lib-class) {), we should move the :global(....) selector to the shared/style.scss file without using global and probably ignore this whole file.

Will need to move turning on camelCase rules to a followup. Turns out it involves adjusting quite a few files :)

mattseddon

LGTM but please my opinion is less valuable than @sroy3's.

package.json

mattseddon · 2023-04-23T20:02:50Z

.stylelintrc.js

+module.exports = {
+  extends: 'stylelint-config-standard-scss',
+  rules: {
+    'custom-property-pattern': null,


we can't control other packages but I think we use camelCase, right? We should turn on and exclude files/lines where we can't update. It will probably encourage us to wrap external modules in specific places instead of using them all throughout the code.

mattseddon · 2023-04-23T20:03:27Z

.stylelintrc.js

@@ -0,0 +1,10 @@
+module.exports = {
+  extends: 'stylelint-config-standard-scss',


[Q] Any rules that are missing that we should have turned on?

mattseddon · 2023-04-23T20:06:47Z

.stylelintrc.js

@@ -0,0 +1,10 @@
+module.exports = {


Can we move as set stylelint.config in the .vscode/settings file? or is there another option?

mattseddon · 2023-04-23T20:07:16Z

webview/src/experiments/components/table/styles.module.scss

@@ -1,11 +1,11 @@
+/* stylelint-disable no-descending-specificity */


If it is a useful rule we should fix.

sroy3

This is great. Wish it could have solved more of our problems, but that's definitely a great help in consistency.
I love how it removed duplication and useless style (I would have thought there were more of those). Spotting these things would have been a long process manually.

sroy3 · 2023-04-24T13:19:24Z

.stylelintrc.js

+module.exports = {
+  extends: 'stylelint-config-standard-scss',
+  rules: {
+    'custom-property-pattern': null,


All kebab-case rules should be inside :global(...). Looking at stylelint configure docs, I've stumbled upon this:

{ "rules": { "selector-pseudo-class-no-unknown": [ true, { "ignorePseudoClasses": ["global"] } ] } }

Not sure if it applies in this case, but if we can ignore global we could set the rule correctly to camelCase.

On another note (out of scope), unless it's really specific to the current CSS module (.myModuleBlock :global(.lib-class) {), we should move the :global(....) selector to the shared/style.scss file without using global and probably ignore this whole file.

sroy3 · 2023-04-24T13:21:18Z

webview/src/experiments/components/table/styles.module.scss

@@ -1,11 +1,11 @@
+/* stylelint-disable no-descending-specificity */


I think it would be really good to fix these. It might be too big for this PR, but a follow up just for this would be great.

webview/src/shared/variables.scss

sroy3 · 2023-04-24T13:49:14Z

.stylelintrc.js

@@ -0,0 +1,10 @@
+module.exports = {
+  extends: 'stylelint-config-standard-scss',


I think some of the rules for "Max and min" would be useful for us as most of our current problems originate from that.

max-nesting-depth
selector-max-attribute
selector-max-class
selector-max-combinator
selector-max-type <- wondering if setting this to 0 would work
selector-max-specificity

* move stylelintrc.js to be inside of webview * improve stylelintrc

julieg18 · 2023-04-25T17:10:31Z

Resolved some of the comments! What's left:

To do before merging

Fix new stylelint errors that popped up when I merged to latest version of main

To do in followups (will create an issue)

Enable camelCase selector rules (Add Stylelint #3736 (comment))
Move stylelint packages into webview/node_modules instead of the root /node_modules (Add Stylelint #3736 (comment))
Remove table/styles disabled comments (Add Stylelint #3736 (comment))

I'll wait a bit before merging to make sure no one wants any of the "followup" comments to be taken care of before we merge!

codeclimate · 2023-04-25T20:00:53Z

Code Climate has analyzed commit 502acf0 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.7% (0.0% change).

View more on Code Climate.

Try out stylelint

e745065

julieg18 added the 🏠 housekeeping label Apr 21, 2023

julieg18 self-assigned this Apr 21, 2023

Merge branch 'main' into add-stylelint

6fdef6e

julieg18 commented Apr 21, 2023

View reviewed changes

julieg18 marked this pull request as ready for review April 21, 2023 20:33

julieg18 requested review from mattseddon and sroy3 as code owners April 21, 2023 20:33

mattseddon approved these changes Apr 23, 2023

View reviewed changes

sroy3 approved these changes Apr 24, 2023

View reviewed changes

julieg18 and others added 3 commits April 25, 2023 10:09

Merge branch 'main' into add-stylelint

30de9ce

Resolve some comments

3e14e8c

* move stylelintrc.js to be inside of webview * improve stylelintrc

Merge branch 'main' into add-stylelint

0927e48

julieg18 and others added 2 commits April 25, 2023 14:50

Merge branch 'main' into add-stylelint

8e27f84

Fix linting

502acf0

julieg18 enabled auto-merge (squash) April 25, 2023 19:54

julieg18 merged commit 16dacc7 into main Apr 25, 2023

julieg18 deleted the add-stylelint branch April 25, 2023 20:08

julieg18 mentioned this pull request Apr 25, 2023

Improve project's CSS #3760

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Stylelint #3736

Add Stylelint #3736

julieg18 commented Apr 21, 2023 •

edited

Loading

julieg18 Apr 21, 2023

mattseddon Apr 23, 2023

mattseddon Apr 23, 2023

mattseddon Apr 24, 2023

julieg18 Apr 25, 2023 •

edited

Loading

julieg18 Apr 21, 2023

julieg18 Apr 21, 2023

mattseddon Apr 23, 2023

sroy3 Apr 24, 2023

julieg18 Apr 24, 2023

julieg18 Apr 21, 2023

mattseddon Apr 23, 2023

sroy3 Apr 24, 2023

julieg18 Apr 25, 2023

sroy3 Apr 25, 2023

julieg18 Apr 21, 2023

mattseddon Apr 23, 2023

sroy3 Apr 24, 2023

julieg18 Apr 25, 2023

mattseddon left a comment

mattseddon Apr 23, 2023

mattseddon Apr 23, 2023

mattseddon Apr 23, 2023

mattseddon Apr 23, 2023

sroy3 left a comment

sroy3 Apr 24, 2023

sroy3 Apr 24, 2023

sroy3 Apr 24, 2023

julieg18 commented Apr 25, 2023 •

edited

Loading

codeclimate bot commented Apr 25, 2023

		@@ -1,11 +1,11 @@
		/* stylelint-disable no-descending-specificity */

		@@ -0,0 +1,10 @@
		module.exports = {
		extends: 'stylelint-config-standard-scss',

Add Stylelint #3736

Add Stylelint #3736

Conversation

julieg18 commented Apr 21, 2023 • edited Loading

Demo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroy3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 commented Apr 25, 2023 • edited Loading

To do before merging

To do in followups (will create an issue)

codeclimate bot commented Apr 25, 2023

julieg18 commented Apr 21, 2023 •

edited

Loading

julieg18 Apr 25, 2023 •

edited

Loading

julieg18 commented Apr 25, 2023 •

edited

Loading