Replace `sass-lint` with `stylelint` #949

gulderov · 2023-08-03T14:32:32Z

Description

sass-lint is an unmaintained project

Please review the migration to stylelint. Which includes the following steps:

Avoid manual changes
The style check script has been changed to yarn lint-style
yarn lint-style --fix was run to auto fix everything possible

Issues Resolved

resolves #119

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

bump stylelint-config-standard-scss: 10.0.0 Signed-off-by: Danila Gulderov <[email protected]>

Signed-off-by: Danila Gulderov <[email protected]>

BSFishy

Looks pretty good so far, just a few comments I had on first inspection

BSFishy · 2023-08-03T18:57:11Z

.stylelintrc.yml

+  # recommended to turn off descending specificity since we use a lot of nesting:
+  # https://stylelint.io/user-guide/rules/list/no-descending-specificity/
+  no-descending-specificity: null


It might be nice to have a selector ordering rule. We should look into this further to see how much work it would be and if we should do it in the first place

We could handle it as separate task

BSFishy · 2023-08-03T18:58:32Z

src-docs/src/components/guide_components.scss

@@ -177,7 +179,7 @@ $elasticLogoTextDark: #1C1E23;
 }

 .guideDemo__ghostBackground {
-  @if (lightness($ouiTextColor) < 50) {
+  @if  lightness($ouiTextColor) < 50 {


Why the extra space here? There seem to be a lot of these? Is it just an artifact of yarn lint-sass-fix?

Suggested change

@if lightness($ouiTextColor) < 50 {

@if lightness($ouiTextColor) < 50 {

It is artifact from yarn lint-style --fix. I was wondering to fix it with prettier. But we need to update prettier first and that is separate task.

I tend to avoid manual intervention and keep it as is for now. Prettier would reformat it later

BSFishy · 2023-08-03T18:59:00Z

src-docs/src/theme_dark.scss

@@ -10,9 +10,8 @@
 */

 // sass-lint:disable no-url-domains, no-url-protocols


Would love to get rid of old sass-lint disables

sass-lint has been removed from the codebase

Signed-off-by: Danila Gulderov <[email protected]>

…earch-project#119) Signed-off-by: Danila Gulderov <[email protected]>

Signed-off-by: Danila Gulderov <[email protected]>

BSFishy · 2023-08-05T16:23:44Z

https://github.com/opensearch-project/oui/actions/runs/5759821731/job/15628275259?pr=949#step:5:13

Do you know why the linter is failing in CI? Does it need to be rerun?

Signed-off-by: Danila Gulderov <[email protected]>

gulderov · 2023-08-08T08:04:15Z

@BSFishy I removed the quotes to try and fix CI

Signed-off-by: Danila Gulderov <[email protected]>

BSFishy · 2023-09-14T16:23:24Z

Love to see the CI is passing, gives me a lot more confidence in this. Will re-review and approve if everything looks good

BSFishy

Overall looks good, will pull down and play around with it then approve

BSFishy · 2023-09-14T16:29:50Z

package.json

+    "lint-style": "stylelint \"{src,src-docs}/**/*.scss\" --max-warnings 0",
+    "lint-style-fix": "yarn lint-style --fix",


Curious if this will cause anything unintended to break. All CI passes, but I wonder if anyone else is using these commands that we don't know about. Either way, I don't think it's something we really need to worry about for breaking things ~ this is more meant for dev anyway

BSFishy · 2023-09-14T16:32:01Z

package.json

@@ -216,6 +210,7 @@
    "moment-timezone": "^0.5.41",
    "node-sass": "npm:@amoo-miki/[email protected]",
    "pegjs": "^0.10.0",
+    "postcss": "^8.4.28",


I am astonished that we were using postcss without specifying it as a dep 🤦

BSFishy

Just a few minor final things

BSFishy · 2023-09-14T16:41:42Z

.stylelintignore

+build
+target


We have a few artifact directories:

dist docs es lib test-env types

Additionally, would node_modules be a good choice to be here?

BSFishy · 2023-09-14T16:42:30Z

.stylelintrc.yml

+  # while we still use node-sass, only legacy rgb() notation is allowed
+  color-function-notation: "legacy"


Will need to followup on this for #1001

gulderov added 11 commits August 3, 2023 16:00

bump stylelint: 15.10.2 (opensearch-project#119)

f557ebb

bump stylelint-config-standard-scss: 10.0.0 Signed-off-by: Danila Gulderov <[email protected]>

bump postcss: 8.4.27 (opensearch-project#119)

7a9de5e

Signed-off-by: Danila Gulderov <[email protected]>

stylelint configs (opensearch-project#119)

62460ed

Signed-off-by: Danila Gulderov <[email protected]>

stylelint auto fix (opensearch-project#119)

b6366ab

Signed-off-by: Danila Gulderov <[email protected]>

remove stylelint config (opensearch-project#119)

a3b0e4c

Signed-off-by: Danila Gulderov <[email protected]>

stylelint config cleanup (opensearch-project#119)

7d80460

Signed-off-by: Danila Gulderov <[email protected]>

stylelint fix (opensearch-project#119)

b226e9c

Signed-off-by: Danila Gulderov <[email protected]>

stylelint fix src-docs (opensearch-project#119)

4bb9391

Signed-off-by: Danila Gulderov <[email protected]>

stylelint fix (opensearch-project#119)

ae29559

Signed-off-by: Danila Gulderov <[email protected]>

sass-lint cleanup (opensearch-project#119)

60a5680

Signed-off-by: Danila Gulderov <[email protected]>

scripts fix (opensearch-project#119)

ba7f671

Signed-off-by: Danila Gulderov <[email protected]>

gulderov requested review from seanneumann, AMoo-Miki, rednaksi91, KrooshalUX, BSFishy, bandinib-amzn, ashwin-pc, joshuarrrr, andreymyssak and SergeyMyssak as code owners August 3, 2023 14:32

github-actions bot added the first-time-contributor label Aug 3, 2023

gulderov added 3 commits August 3, 2023 17:40

Merge branch 'main' into stylelint

417dda4

Signed-off-by: Danila Gulderov <[email protected]>

stylelint fix (opensearch-project#119)

d06bbaa

Signed-off-by: Danila Gulderov <[email protected]>

removal of saas-lint comments (opensearch-project#119)

e3b0ff6

Signed-off-by: Danila Gulderov <[email protected]>

gulderov changed the title ~~Replace sass-lint with stylelint~~ Draft: Replace sass-lint with stylelint Aug 3, 2023

BSFishy reviewed Aug 3, 2023

View reviewed changes

gulderov added 3 commits August 4, 2023 09:31

limit script to lint:style only (opensearch-project#119)

222f61b

Signed-off-by: Danila Gulderov <[email protected]>

prefer stylelint-disable-next-line over stylelint-disable-line (opens…

c725ad3

…earch-project#119) Signed-off-by: Danila Gulderov <[email protected]>

sass-lint cleanup (opensearch-project#119)

6d6e6f7

Signed-off-by: Danila Gulderov <[email protected]>

rename scripts (opensearch-project#119)

5154f25

Signed-off-by: Danila Gulderov <[email protected]>

gulderov changed the title ~~Draft: Replace sass-lint with stylelint~~ Replace sass-lint with stylelint Aug 4, 2023

gulderov added 2 commits August 4, 2023 10:42

add script lint-style-fix (opensearch-project#119)

b120590

Signed-off-by: Danila Gulderov <[email protected]>

update changelog (opensearch-project#119)

0e5ba98

Signed-off-by: Danila Gulderov <[email protected]>

gulderov mentioned this pull request Aug 4, 2023

Migrate to Dart Sass #942

Closed

7 tasks

quotes replace

945d2cb

Signed-off-by: Danila Gulderov <[email protected]>

quotes (opensearch-project#119)

16e2dad

Signed-off-by: Danila Gulderov <[email protected]>

AMoo-Miki self-assigned this Aug 24, 2023

gulderov added 3 commits August 24, 2023 23:16

packages update (opensearch-project#119)

870a9db

Signed-off-by: Danila Gulderov <[email protected]>

Merge branch 'main' into stylelint (opensearch-project#119)

8f0d289

Signed-off-by: Danila Gulderov <[email protected]>

Merge branch 'main' into stylelint

ec4765c

Signed-off-by: Danila Gulderov <[email protected]>

gulderov requested a review from BSFishy September 6, 2023 06:34

gulderov and others added 3 commits September 6, 2023 09:38

Merge branch 'main' into stylelint

0904cb9

Signed-off-by: Danila Gulderov <[email protected]>

stylelint fix (opensearch-project#119)

d2eb2e1

Signed-off-by: Danila Gulderov <[email protected]>

Merge branch 'main' into stylelint

b07f171

BSFishy reviewed Sep 14, 2023

View reviewed changes

BSFishy mentioned this pull request Sep 14, 2023

Audit scripts #573

Open

26 tasks

BSFishy mentioned this pull request Oct 9, 2023

Audit unused packages #595

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `sass-lint` with `stylelint` #949

Replace `sass-lint` with `stylelint` #949

gulderov commented Aug 3, 2023 •

edited

Loading

BSFishy left a comment

BSFishy Aug 3, 2023

gulderov Aug 4, 2023

BSFishy Aug 3, 2023

gulderov Aug 4, 2023 •

edited

Loading

BSFishy Aug 3, 2023

gulderov Aug 4, 2023 •

edited

Loading

BSFishy commented Aug 5, 2023

gulderov commented Aug 8, 2023

BSFishy commented Sep 14, 2023

BSFishy left a comment

BSFishy Sep 14, 2023

BSFishy Sep 14, 2023

BSFishy left a comment

BSFishy Sep 14, 2023

BSFishy Sep 14, 2023

	@if lightness($ouiTextColor) < 50 {
	@if lightness($ouiTextColor) < 50 {

		@@ -10,9 +10,8 @@
		*/

		// sass-lint:disable no-url-domains, no-url-protocols

		"lint-style": "stylelint \"{src,src-docs}/*/.scss\" --max-warnings 0",
		"lint-style-fix": "yarn lint-style --fix",

		# while we still use node-sass, only legacy rgb() notation is allowed
		color-function-notation: "legacy"

		build
		target

Replace sass-lint with stylelint #949

Are you sure you want to change the base?

Replace sass-lint with stylelint #949

Conversation

gulderov commented Aug 3, 2023 • edited Loading

Description

Issues Resolved

Check List

BSFishy 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

gulderov Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gulderov Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

BSFishy commented Aug 5, 2023

gulderov commented Aug 8, 2023

BSFishy commented Sep 14, 2023

BSFishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BSFishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replace `sass-lint` with `stylelint` #949

Replace `sass-lint` with `stylelint` #949

gulderov commented Aug 3, 2023 •

edited

Loading

gulderov Aug 4, 2023 •

edited

Loading

gulderov Aug 4, 2023 •

edited

Loading