Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stylelint hook and config #1821

Closed
wants to merge 2 commits into from
Closed

Conversation

gabalafou
Copy link
Collaborator

This PR adds the stylelint hook and a config file

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I'd prefer the addition of the config in a separate PR from the linter-induced changes, but that seems hard given our pre-commit CI setup. So in this case I would be in favor of

  1. just before merging, rebase so that there are exactly 2 commits here (one to add tooling, one for linter changes)
  2. temporarily enable rebase-merges, and merge via rebase
  3. in subsequent PR, add the second commit to .git-blame-ignore-revs

Comment on lines +40 to 41
appearance: none;
appearance: none;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant after linter fix

Comment on lines 11 to +15
user-select: none;
-webkit-user-select: text; /* Safari fallback only */
-webkit-user-select: none; /* Chrome/Safari */
-moz-user-select: none; /* Firefox */
-ms-user-select: none; /* IE10+ */
user-select: text; /* Safari fallback only */
user-select: none; /* Chrome/Safari */
user-select: none; /* Firefox */
user-select: none; /* IE10+ */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant after lint

Comment on lines +35 to +39
--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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be OK, but I'm paranoid about preserving font name capitalization (legacy of many font-related LaTeX compilation errors). Is there an automated way we can we check this still works as designed?

@gabalafou gabalafou mentioned this pull request May 17, 2024
@gabalafou
Copy link
Collaborator Author

Ideally I'd prefer the addition of the config in a separate PR from the linter-induced changes, but that seems hard given our pre-commit CI setup.

Okay, see the note I wrote in #1823.

@gabalafou
Copy link
Collaborator Author

Closing in favor of #1823.

@gabalafou gabalafou closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants