Add style linter to pre-commit, and run #1764
Conversation
|
Note: this did not pick up the bug fixed in #1761. Why? A selector of the form |
|
Should this PR be separated? One commit that adds stylelint, another that runs the linter? |
| // Thickness of the underline for links | ||
| // the default will be either: | ||
| // - 1px | ||
| // - 0.0625rem if it's thicker than 1px because the user has changed the text | ||
| // size in their browser | ||
| $link-underline-thickness: unquote("max(1px, .0625rem)") !default; | ||
| $link-underline-thickness: string.unquote("max(1px, .0625rem)") !default; |
There was a problem hiding this comment.
is string.unquote even necessary here? I don't understand what it's doing
There was a problem hiding this comment.
Yes, the unquote is necessary when compiling this will result in max(1px, .0625rem) which is a valid identifier.
| user-select: text; /* Safari fallback only */ /* Chrome/Safari */ /* Firefox */ | ||
| user-select: none; /* IE10+ */ |
There was a problem hiding this comment.
This might need some modification
There was a problem hiding this comment.
Think it should be user-select: text; right? Unless I am missing something https://developer.mozilla.org/en-US/docs/Web/CSS/user-select#browser_compatibility
| @include icon-navbar-hover; | ||
| @include focus-indicator; | ||
| } | ||
|
|
||
| // Hide the navbar header items on mobile because they're in the sidebar | ||
| // stylelint-disable-next-line no-duplicate-selectors |
There was a problem hiding this comment.
Didn't feel easy and safe to move these rules so I left the duplicate selector
| @@ -163,6 +166,7 @@ | |||
| */ | |||
|
|
|||
| // Hide the header items on mobile | |||
| // stylelint-disable-next-line no-duplicate-selectors | |||
There was a problem hiding this comment.
Didn't feel easy and safe to move these rules so I left the duplicate selector
| --pst-font-family-base-system: -apple-system, blinkmacsystemfont, segoe ui, | ||
| "Helvetica Neue", arial, sans-serif, apple color emoji, segoe ui emoji, | ||
| segoe ui symbol; | ||
| --pst-font-family-monospace-system: "SFMono-Regular", menlo, consolas, monaco, | ||
| liberation mono, lucida console, monospace; |
There was a problem hiding this comment.
No idea why the linter wanted to lower case all of the unquoted font names 🤷♂️
There was a problem hiding this comment.
Does it work with all lowercase?
This seems like a hard rule of making all nonn-quoted strings a valid identifier, but I am not sure this applies to Fonts.
The linter is being pedantic (as linters do). Browsers accept both |
trallard
added
the
kind: maintenance
There was a problem hiding this comment.
Thanks @gabalafou left a couple of comments. I agree it might be worth splitting this in 1) adding the linter 2) run the linter as we have a whole lot of changes here
| user-select: text; /* Safari fallback only */ /* Chrome/Safari */ /* Firefox */ | ||
| user-select: none; /* IE10+ */ |
There was a problem hiding this comment.
Think it should be user-select: text; right? Unless I am missing something https://developer.mozilla.org/en-US/docs/Web/CSS/user-select#browser_compatibility
| img:not(.only-dark, .dark-light) { | ||
| filter: brightness(0.8) contrast(1.2); | ||
| } | ||
|
|
There was a problem hiding this comment.
For another time, this would need revisiting with all the a11y work in flight
| --pst-font-family-base-system: -apple-system, blinkmacsystemfont, segoe ui, | ||
| "Helvetica Neue", arial, sans-serif, apple color emoji, segoe ui emoji, | ||
| segoe ui symbol; | ||
| --pst-font-family-monospace-system: "SFMono-Regular", menlo, consolas, monaco, | ||
| liberation mono, lucida console, monospace; |
There was a problem hiding this comment.
Does it work with all lowercase?
This seems like a hard rule of making all nonn-quoted strings a valid identifier, but I am not sure this applies to Fonts.
|
@gabalafou Before merging this, it would be great if you could rebase and then add a third commit adding the commit corresponding to "fix errors emitted by style linter" to We will need to enable and use "Create a merge commit". Otherwise the commit in |
|
@jarrodmillman will do, thanks for the tip! |
@jarrodmillman since we typically squash-merge, I would suggest this workflow instead:
The advantage is that whoever does the merging doesn't need to have the repo-admin permissions to change our merge strategy, and doesn't need to remember to change it back afterwards. |
|
This work is picked up in #1823. |
No description provided.