Skip to content

Commit

Permalink
Update Stylelint to v14, extend SCSS plugins, remove primer-* configs…
Browse files Browse the repository at this point in the history
…, resolve issues (just-the-docs#821)

This is a catch-all PR that modernizes and updates our Stylelint config, and resolves all open issues. This is a pretty big change - so I want to update all of our related dependencies in lockstep.

In particular, this PR

- [x] updates stylelint to `v14`
- [x] adds in the standard stylelint config for SCSS (`stylelint-config-standard-scss`)
- [x] swaps out `stylelint-config-prettier` for `stylelint-config-prettier-scss`
- [x] ~~properly update `@primer`-related plugins:~~ completely remove `primer` from our configuration
- [x] autofix, manually resolve, or disable all newly-introduced lint errors; **I've avoided manually resolving errors that would be a behavioural change**
- [x] re-runs `npm run format`

See the "next steps" section on some extra thoughts on disabling errors.

(implicitly, I'm also using node 16/the new package-lock format).

### disabling rules and next steps

I've introduced several new disabled rules. Let me quickly explain what's going on; there are two categories of rules I've disabled:

1. rules that were temporary disables; they were frequent enough that I couldn't manually resolve them, but should be simple. **I plan on opening issues to re-enable each of these rules**, just after this PR
    - `declaration-block-no-redundant-longhand-properties`: this is just tedious and error-prone
    - `no-descending-specificity`: this one is tricky since it could have impacts on the cascade (though that seems unlikely)
    - `scss/no-global-function-names`: I think we need to [import map and then use `map.get`](https://stackoverflow.com/questions/64210390/sass-map-get-doesnt-work-map-get-does-what-gives), but I'll leave this as out of scope for now
2. rules that are long-term disables; due to the SASS-based nature of our theme, I think we'll keep these in limbo
    - `alpha-value-notation` causes problems with SASS using the `modern` syntax - literals like `50%` are not properly interpolated, and they cause formatting issues on the site
    - `color-function-notation` also causes problems with SASS, but in this case the `modern` syntax breaks SASS compilation; we're not alone (see this [SO post](https://stackoverflow.com/questions/71805735/error-function-rgb-is-missing-argument-green-in-sass)). 

In addition, we have many inline `stylelint-disable` comments. I'd open a separate issue to audit them, especially since I think some disables are unnecessary.

### on Primer 

**note: there hasn't been much other discussion, so I'm going to remove primer's stylelint config.**

If I do add `@primer/stylelint-config`, I get *a ton* of errors about now using `@primer`'s in-built SCSS variables. I imagine that we probably won't want to use these presets (though I could be wrong). In that case, I think we could either:

1. disable all of those rules
4. not use `@primer/stylelint-config`, since we're not actually using primer, and shift back to the standard SCSS config provided by Stylelint

~~Any thoughts here? I also don't have the original context as to why we do use the primer rules, perhaps @pmarsceill can chime in?~~
  • Loading branch information
mattxwang authored Jul 25, 2022
1 parent a6f0452 commit c2ec3d8
Show file tree
Hide file tree
Showing 31 changed files with 3,474 additions and 5,983 deletions.
54 changes: 26 additions & 28 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
@@ -1,37 +1,35 @@
// For format details, see https://aka.ms/devcontainer.json. For config options, see the README at:
// https://github.com/microsoft/vscode-dev-containers/tree/v0.224.2/containers/jekyll
{
"name": "Just the docs",
"build": {
"dockerfile": "Dockerfile",
"args": {
// Update 'VARIANT' to pick a Debian OS version: bullseye, buster
// Use bullseye when on local arm64/Apple Silicon.
"VARIANT": "bullseye",
// Enable Node.js: pick the latest LTS version
"NODE_VERSION": "lts/*"
}
},
"name": "Just the docs",
"build": {
"dockerfile": "Dockerfile",
"args": {
// Update 'VARIANT' to pick a Debian OS version: bullseye, buster
// Use bullseye when on local arm64/Apple Silicon.
"VARIANT": "bullseye",
// Enable Node.js: pick the latest LTS version
"NODE_VERSION": "lts/*"
}
},

// Set *default* container specific settings.json values on container create.
"settings": {},
// Set *default* container specific settings.json values on container create.
"settings": {},

// Add the IDs of extensions you want installed when the container is created.
"extensions": [
"GitHub.vscode-pull-request-github"
],
// Add the IDs of extensions you want installed when the container is created.
"extensions": ["GitHub.vscode-pull-request-github"],

// Use 'forwardPorts' to make a list of ports inside the container available locally.
"forwardPorts": [
// Jekyll server
4000,
// Live reload server
35729
],
// Use 'forwardPorts' to make a list of ports inside the container available locally.
"forwardPorts": [
// Jekyll server
4000,
// Live reload server
35729
],

// Use 'postCreateCommand' to run commands after the container is created.
"postCreateCommand": "sh .devcontainer/post-create.sh",
// Use 'postCreateCommand' to run commands after the container is created.
"postCreateCommand": "sh .devcontainer/post-create.sh",

// Comment out to connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root.
"remoteUser": "vscode"
// Comment out to connect as root instead. More info: https://aka.ms/vscode-remote/containers/non-root.
"remoteUser": "vscode"
}
12 changes: 10 additions & 2 deletions .stylelintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,17 @@
"assets/css/just-the-docs-dark.scss",
"_sass/vendor/**/*.scss"
],
"extends": ["stylelint-config-primer", "stylelint-config-prettier"],
"extends": [
"stylelint-config-standard-scss",
"stylelint-config-prettier-scss"
],
"plugins": ["stylelint-prettier"],
"rules": {
"prettier/prettier": true
"prettier/prettier": true,
"alpha-value-notation": null,
"color-function-notation": null,
"declaration-block-no-redundant-longhand-properties": null,
"no-descending-specificity": null,
"scss/no-global-function-names": null
}
}
50 changes: 25 additions & 25 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
@@ -1,26 +1,26 @@
{
// See https://go.microsoft.com/fwlink/?LinkId=733558
// for the documentation about the tasks.json format
"version": "2.0.0",
"tasks": [
{
"label": "Serve",
"type": "shell",
"command": "bundle exec jekyll serve --livereload",
"group": {
"kind": "test",
"isDefault": true
},
"isBackground": true,
},
{
"label": "Build",
"type": "shell",
"command": "bundle exec jekyll build",
"group": {
"kind": "build",
"isDefault": true
},
}
]
}
// See https://go.microsoft.com/fwlink/?LinkId=733558
// for the documentation about the tasks.json format
"version": "2.0.0",
"tasks": [
{
"label": "Serve",
"type": "shell",
"command": "bundle exec jekyll serve --livereload",
"group": {
"kind": "test",
"isDefault": true
},
"isBackground": true
},
{
"label": "Build",
"type": "shell",
"command": "bundle exec jekyll build",
"group": {
"kind": "build",
"isDefault": true
}
}
]
}
3 changes: 1 addition & 2 deletions _sass/base.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//
// Base element style overrides
//
// stylelint-disable selector-no-type, selector-max-type, selector-max-specificity, selector-max-id

* {
Expand All @@ -14,6 +12,7 @@

html {
@include fs-4;

scroll-behavior: smooth;
}

Expand Down
2 changes: 0 additions & 2 deletions _sass/buttons.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//
// Buttons and things that look like buttons
//
// stylelint-disable color-named

.btn {
Expand Down
5 changes: 2 additions & 3 deletions _sass/code.scss
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
//
// Code and syntax highlighting
//
// stylelint-disable selector-no-qualifying-type, declaration-block-semicolon-newline-after,declaration-block-single-line-max-declarations, selector-no-type, selector-max-type
// stylelint-disable selector-no-qualifying-type, declaration-block-semicolon-newline-after,declaration-block-single-line-max-declarations, selector-no-type, selector-max-type, scss/comment-no-empty

// {% raw %}

Expand Down Expand Up @@ -100,6 +98,7 @@ figure.highlight {
td,
pre {
@include fs-2;

min-width: 0;
padding: 0;
background-color: $code-background-color;
Expand Down
6 changes: 1 addition & 5 deletions _sass/color_schemes/dark.scss
Original file line number Diff line number Diff line change
@@ -1,28 +1,24 @@
$body-background-color: $grey-dk-300;
$sidebar-color: $grey-dk-300;
$border-color: $grey-dk-200;

$body-text-color: $grey-lt-300;
$body-heading-color: $grey-lt-000;
$nav-child-link-color: $grey-dk-000;
$search-result-preview-color: $grey-dk-000;

$link-color: $blue-000;
$btn-primary-color: $blue-200;
$base-button-color: $grey-dk-250;

$search-background-color: $grey-dk-250;
$table-background-color: $grey-dk-250;
$feedback-color: darken($sidebar-color, 3%);

//
// The following highlight theme is more legible than that used for the light color scheme
//

// @import "./vendor/OneDarkJekyll/syntax-one-dark";
// $code-background-color: #282c34;

@import "./vendor/OneDarkJekyll/syntax-one-dark-vivid";

$code-background-color: #31343f;

// @import "./vendor/OneDarkJekyll/syntax-firewatch";
Expand Down
5 changes: 3 additions & 2 deletions _sass/content.scss
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
@charset "UTF-8";

//
// Styles for rendered markdown in the .main-content container
//
// stylelint-disable selector-no-type, max-nesting-depth, selector-max-compound-selectors, selector-max-type, selector-max-specificity, selector-max-id

.main-content {
Expand Down Expand Up @@ -91,6 +89,7 @@
.task-list-item-checkbox {
margin-right: 0.6em;
margin-left: -1.4em;

// The same margin-left is used above for ul > li::before
}

Expand All @@ -116,6 +115,7 @@
grid-column: 1;
font-weight: 500;
text-align: right;

&::after {
content: ":";
}
Expand All @@ -125,6 +125,7 @@
grid-column: 2;
margin-bottom: 0;
margin-left: 1em;

blockquote,
div,
dl,
Expand Down
3 changes: 1 addition & 2 deletions _sass/labels.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//
// Labels (not the form kind)
//

.label,
.label-blue {
Expand All @@ -16,6 +14,7 @@
vertical-align: middle;
background-color: $blue-100;
@include fs-2;

border-radius: 12px;
}

Expand Down
5 changes: 3 additions & 2 deletions _sass/layout.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//
// The basic two column layout
//

.side-bar {
z-index: 0;
Expand Down Expand Up @@ -40,6 +38,7 @@

.main-content-wrap {
@include container;

padding-top: $gutter-spacing-sm;
padding-bottom: $gutter-spacing-sm;

Expand Down Expand Up @@ -111,6 +110,7 @@

.site-title {
@include container;

flex-grow: 1;
display: flex;
height: 100%;
Expand Down Expand Up @@ -184,6 +184,7 @@ body {

.site-footer {
@include container;

position: absolute;
bottom: 0;
left: 0;
Expand Down
6 changes: 1 addition & 5 deletions _sass/modules.scss
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
//
// Import external dependencies
//
@import "./vendor/normalize.scss/normalize.scss";
@import "./vendor/normalize.scss/normalize";

//
// Modules
//
@import "./base";
@import "./layout";
@import "./content";
Expand Down
4 changes: 2 additions & 2 deletions _sass/navigation.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//
// Main nav, breadcrumb, etc...
//
// stylelint-disable selector-no-type, max-nesting-depth, selector-max-compound-selectors, selector-max-type, selector-max-specificity

.nav-list {
Expand All @@ -11,6 +9,7 @@

.nav-list-item {
@include fs-4;

position: relative;
margin: 0;

Expand Down Expand Up @@ -71,6 +70,7 @@
@if $nav-list-expander-right {
right: 0;
}

width: $nav-list-item-height-sm;
height: $nav-list-item-height-sm;
padding-top: #{$nav-list-item-height-sm / 4};
Expand Down
2 changes: 1 addition & 1 deletion _sass/print.scss
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// stylelint-disable selector-max-specificity, selector-max-id, selector-max-type, selector-no-qualifying-type, primer/no-override,
// stylelint-disable selector-max-specificity, selector-max-id, selector-max-type, selector-no-qualifying-type

@media print {
.site-footer,
Expand Down
2 changes: 0 additions & 2 deletions _sass/search.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//
// Search input and autocomplete
//

.search {
position: relative;
Expand Down
8 changes: 4 additions & 4 deletions _sass/support/_functions.scss
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
@function rem($size, $unit: "") {
$remSize: $size / $root-font-size;
$rem-size: $size / $root-font-size;

@if ($unit == false) {
@return #{$remSize};
@if $unit == false {
@return #{$rem-size};
} @else {
@return #{$remSize}rem;
@return #{$rem-size}rem;
}
}
Loading

0 comments on commit c2ec3d8

Please sign in to comment.