🪟🧹 Stylelint #15452

krishnaglick · 2022-08-09T14:35:48Z

Adds stylelint to the app.

Some relevant reading regarding our use of extends: http://8gramgorilla.com/mastering-sass-extends-and-placeholders/

krishnaglick · 2022-08-10T14:22:51Z

airbyte-webapp/.stylelintrc

+    "font-family-name-quotes": null,
+    "scss/dollar-variable-empty-line-before": null,
+    "scss/dollar-variable-pattern": ".*",
+    "scss/percent-placeholder-pattern": ".*",


This enforces kebab case in s/css class names, which doesn't work well for us thanks to modules.

krishnaglick · 2022-08-10T14:22:59Z

airbyte-webapp/build.gradle

@@ -12,7 +12,8 @@ def commonConfigs = [
    'package.json',
    'package-lock.json',
    'tsconfig.json',
-    '.prettierrc.js'
+    '.prettierrc.js',
+    '.stylelintrc'


Can remove, felt it didn't hurt to add.

krishnaglick · 2022-08-10T14:24:58Z

airbyte-webapp/package.json

    "license-check": "node ./scripts/license-check.js",
    "generate-client": "orval",
-    "validate-links": "ts-node --skip-project ./scripts/validate-links.ts"
+    "validate-links": "ts-node --skip-project ./scripts/validate-links.ts",
+    "stylelint-check": "stylelint-config-prettier-scss-check"


Not sure if we should add this to a pipeline somewhere, or just ignore.

We should def run that on CI, since it's otherwise not really linting anything. For that we should add a new task to our build.gradle file (you can see similar type: NpmTask in there), and make the gradle check step depend on this (line 108 you can just add a new task that should run for checks).

We should make also sure that the task running on CI fails if it would restyle any SCSS file, i.e. make git dirty by running. Ideally we see if any of the stylelint commands actually has a flag to automatically fail if it needs to touch a SCSS file (and separate this here into two tasks: one that does reformat for running locally, and one for CI that fails when it would reformat).

If it doesn't exist we'll need to build into gradle a check that validates that no file got touched after running the stylelint task.

krishnaglick · 2022-08-10T14:25:21Z

airbyte-webapp/public/index.css

    border-radius: 8px;
-    box-shadow: 0 1px 2px rgba(0, 0, 0, 0.25);
+    box-shadow: 0 1px 2px rgba(0, 0, 0, 25%);


Some of these the linter was able to auto-fix so I let it.

krishnaglick · 2022-08-10T14:25:39Z

airbyte-webapp/public/index.css

@@ -3,6 +3,7 @@ body, html {
    height: 100%;


We may want to yeet this whole file.

krishnaglick · 2022-08-10T14:26:50Z

airbyte-webapp/src/components/JobItem/components/MainInfo.module.scss

      div:hover > div > &,
-      div:hover > div > div > &,
-      div:hover > & {
+      div:hover > div > div > & {


stylelint gets picky about order

krishnaglick · 2022-08-10T14:32:04Z

airbyte-webapp/src/components/base/Switch/Switch.module.scss

@@ -30,45 +62,45 @@
    &:checked + .slider {
      background-color: colors.$blue;

-      &:before {
-        @include knobTransform(right, false);
+      &::before {


::before is the modern syntax

krishnaglick · 2022-08-10T14:32:32Z

airbyte-webapp/src/utils/testutils.tsx

@@ -43,6 +43,7 @@ export async function render<
    renderResult = await rtlRender<Q, Container>(<div>{ui}</div>, { wrapper: Wrapper, ...renderOptions });
  });

+  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion


Just some linting cleanup

krishnaglick · 2022-08-10T14:33:07Z

airbyte-webapp/src/views/Connection/CatalogDiffModal/components/DiffFieldTable.module.scss

@@ -2,7 +2,7 @@
 @use "./DiffSection.module.scss";

 .accordionSubHeader {
-  @extend .sectionSubHeader;
+  @extend %sectionSubHeader;


Can revert, but linter wanted us to use placeholders over classes as classes can result in unintended side effects.

airbyte-webapp/src/views/Connection/ConnectionForm/components/SyncCatalogField.module.scss

teallarson

Seems probably worth getting more than one input, but I'm generally on board with letting a linter think about things so I don't have to.

Here's some more context around the placeholders straight from SASS. I'm not entirely sure from this that it does the same thing?: https://sass-lang.com/documentation/at-rules/extend#placeholder-selectors

airbyte-webapp/.stylelintrc

airbyte-webapp/src/views/Connection/CatalogDiffModal/components/StreamRow.module.scss

timroes · 2022-08-22T16:52:53Z

airbyte-webapp/build.gradle

@@ -104,8 +105,15 @@ task copyNginx(type: Copy) {
    into "build/docker/bin/nginx"
 }

+task lint(type: NpmTask) {
+    dependsOn npmInstall
+    args = ['run', 'lint']


We actually don't need to run lint completely, since Create React App already runs eslint as part of the run build (and thus we'd simply run it twice now). It should be enough just running the stylelint task individually via Gradle.

Should I make an npm run stylelint or should I invoke it directly?

airbyte-webapp/package.json

dizel852

LGTM
Lets wait final approve from @timroes

timroes · 2022-08-25T09:47:30Z

airbyte-webapp/build.gradle

@@ -23,6 +23,7 @@ node {
 npm_run_build {
    inputs.files commonConfigs
    inputs.file '.eslintrc'
+    inputs.file '.stylelintrc'


Sorry, one last thing 😅 But with running stylelint as a separate task, we should move this as an input to the stylelint task below. The npm run build task would have no different output if this file changes.

Fixed I think. You know gradle better than I :)

timroes

One last comment about the gradle build file. Once that is resolved LGTM.

* Stylelint added * lint staged addition * Found the right incantation * WIP * Typescript import * stylelint --fix * Stylelint runs clean! * More linting! * paths are hard * linting * Tim Code Review * lint part of build * Better selector class pattern rule * Tim CR * Linting + --fix * Fixing stylelint issue * Tim CR

krishnaglick added 5 commits August 8, 2022 16:59

Stylelint added

f8c86b6

lint staged addition

9d628a8

Found the right incantation

3336743

WIP

9201a26

Typescript import

a64d973

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Aug 9, 2022

Merge branch 'master' into kc/stylelint

f385c75

krishnaglick linked an issue Aug 9, 2022 that may be closed by this pull request

Add stylelint to the webapp #13230

Closed

krishnaglick added 2 commits August 9, 2022 14:42

stylelint --fix

3812045

Merge branch 'master' into kc/stylelint

f5ae729

krishnaglick added area/frontend technical-debt issues to fix code smell and removed area/platform issues related to the platform labels Aug 9, 2022

Stylelint runs clean!

ef91a4a

github-actions bot added the area/platform issues related to the platform label Aug 9, 2022

krishnaglick added 5 commits August 9, 2022 15:18

Merge branch 'master' into kc/stylelint

70c46da

More linting!

1a82607

paths are hard

b5b2a55

linting

3fa9522

Merge branch 'master' into kc/stylelint

bfecba9

krishnaglick commented Aug 10, 2022

View reviewed changes

krishnaglick marked this pull request as ready for review August 10, 2022 14:34

krishnaglick requested a review from a team as a code owner August 10, 2022 14:34

krishnaglick changed the title ~~Stylelint~~ 🧹 Stylelint Aug 10, 2022

Merge branch 'master' of github.com:airbytehq/airbyte into kc/stylelint

3cce9bb

teallarson approved these changes Aug 11, 2022

View reviewed changes

krishnaglick changed the title ~~🧹 Stylelint~~ 🪟🧹 Stylelint Aug 11, 2022

timroes reviewed Aug 11, 2022

View reviewed changes

airbyte-webapp/.stylelintrc Show resolved Hide resolved

timroes reviewed Aug 11, 2022

View reviewed changes

airbyte-webapp/src/views/Connection/CatalogDiffModal/components/StreamRow.module.scss Outdated Show resolved Hide resolved

Tim Code Review

92bb089

krishnaglick requested a review from timroes August 12, 2022 14:17

krishnaglick added 3 commits August 16, 2022 10:30

Merge branch 'master' of github.com:airbytehq/airbyte into kc/stylelint

f88976d

lint part of build

2e029d5

Better selector class pattern rule

b20f846

krishnaglick requested a review from dizel852 August 22, 2022 14:18

Merge branch 'master' into kc/stylelint

9837b37

timroes reviewed Aug 22, 2022

View reviewed changes

airbyte-webapp/package.json Outdated Show resolved Hide resolved

krishnaglick added 2 commits August 22, 2022 13:28

Tim CR

26d1de8

Merge branch 'master' of github.com:airbytehq/airbyte into kc/stylelint

00e32c9

krishnaglick requested a review from timroes August 22, 2022 17:57

Linting + --fix

c0cd023

dizel852 approved these changes Aug 23, 2022

View reviewed changes

krishnaglick added 2 commits August 24, 2022 09:47

Merge branch 'master' of github.com:airbytehq/airbyte into kc/stylelint

e2ce7da

Fixing stylelint issue

216914e

timroes reviewed Aug 25, 2022

View reviewed changes

timroes approved these changes Aug 25, 2022

View reviewed changes

krishnaglick added 4 commits August 25, 2022 09:25

Tim CR

0791762

Merge branch 'master' into kc/stylelint

e74b02d

Merge branch 'master' into kc/stylelint

0e5bf53

Merge branch 'master' into kc/stylelint

0472f6f

krishnaglick merged commit 78ca6c7 into master Aug 29, 2022

krishnaglick deleted the kc/stylelint branch August 29, 2022 16:31

octavia-squidington-iii mentioned this pull request Sep 2, 2022

Bump Airbyte version from 0.40.3 to 0.40.4 #16275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟🧹 Stylelint #15452

🪟🧹 Stylelint #15452

krishnaglick commented Aug 9, 2022 •

edited

Loading

krishnaglick Aug 10, 2022

krishnaglick Aug 10, 2022

krishnaglick Aug 10, 2022

timroes Aug 11, 2022 •

edited

Loading

krishnaglick Aug 10, 2022

krishnaglick Aug 10, 2022

krishnaglick Aug 10, 2022

krishnaglick Aug 10, 2022

krishnaglick Aug 10, 2022

krishnaglick Aug 10, 2022

teallarson left a comment •

edited

Loading

timroes Aug 22, 2022

krishnaglick Aug 22, 2022

dizel852 left a comment

timroes Aug 25, 2022

krishnaglick Aug 25, 2022

timroes left a comment

🪟🧹 Stylelint #15452

🪟🧹 Stylelint #15452

Conversation

krishnaglick commented Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes Aug 11, 2022 • 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

teallarson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dizel852 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

krishnaglick commented Aug 9, 2022 •

edited

Loading

timroes Aug 11, 2022 •

edited

Loading

teallarson left a comment •

edited

Loading