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

docs: add Stylelint configuration and cleanup #16379

Merged
merged 35 commits into from Oct 27, 2022

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Oct 2, 2022

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

What changes did you make? (Give an overview)

Add Stylelint for SCSS files and address the simpler rules in separate commits. Avoided more of the rules that would have funtional changes like the duplicate rules or vendor prefixes

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon build This change relates to ESLint's build process labels Oct 2, 2022
@netlify
Copy link

netlify bot commented Oct 2, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit ccf67a0
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6359893fe4ab7900090755a7
😎 Deploy Preview https://deploy-preview-16379--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@nschonni nschonni changed the title ci: add Stylelint hook for docs docs: add Stylelint configuration and cleanup Oct 2, 2022
@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Oct 2, 2022
docs/src/assets/scss/print.scss Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Oct 3, 2022

@eslint/website-team this is for you folks to decide how to move forward.

kecrily
kecrily approved these changes Oct 4, 2022
Copy link
Member

@kecrily kecrily left a comment

Choose a reason for hiding this comment

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

LGTM, I like all the ways to make the code uniform and neat

docs/package.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@harish-sethuraman harish-sethuraman left a comment

Choose a reason for hiding this comment

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

Shall we also add a lint:fix:scss command too?

@nschonni
Copy link
Contributor Author

nschonni commented Oct 4, 2022

Shall we also add a lint:fix:scss command too?

I've seen you use the lint:fix:* here. I usually find the plain lint:* and fix:* more common in other projects (and less to type 😄)

@harish-sethuraman
Copy link
Member

harish-sethuraman commented Oct 4, 2022

I've found lint:fix being used in most of the projects that I've worked on 😅 and here is an issue regarding standardization in case if you were wondering.

PS: mostly we should avoid manually fixing (running the lint:fix commands) lint issues in case they are auto fixable. We can consider adding pre commit hook just in case.

@nschonni
Copy link
Contributor Author

nschonni commented Oct 4, 2022

PS: mostly we should avoid manually fixing (running the lint:fix commands) lint issues in case they are auto fixable. We can consider adding pre commit hook just in case.

Yeah, I thought about adding the lint-staged for this, but that setup is at the root project, and I figured it was better to keep this dependency inside the docs/ folder. It might still work, I just didn't dig into getting it setup

@@ -15,7 +15,8 @@
"build:sass": "sass --style=compressed src/assets/scss:src/assets/css --no-source-map",
"build:eleventy": "npx @11ty/eleventy",
"start": "npm-run-all build:sass --parallel watch:*",
"build": "npm-run-all build:sass build:eleventy images"
"build": "npm-run-all build:sass build:eleventy images",
"test": "stylelint \"**/*.scss\""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"test": "stylelint \"**/*.scss\""
"lint:scss": "stylelint \"**/*.scss\"",
"lint:fix:scss": "npm run lint:scss -- --fix"

@nschonni
Copy link
Contributor Author

nschonni commented Oct 4, 2022

Feel free to push any suggestions to the branch. I've just been flagging suggestions, but you folks can make the final call on which way you want to go.
I split out all the commits separately, but this can definitely be squash/merged in the end, if you want to accept it.

@harish-sethuraman
Copy link
Member

We already have some lint-staged scripts running for files inside the docs directory. We can use the same and add stylelint alone?

eslint/package.json

Lines 36 to 43 in 720ff75

"lint-staged": {
"*.js": "eslint --fix",
"*.md": "markdownlint --fix",
"docs/src/rules/*.md": [
"node tools/fetch-docs-links.js",
"git add docs/src/_data/further_reading_links.json"
],
"docs/**/*.svg": "npx svgo -r --multipass"

@nzakas
Copy link
Member

nzakas commented Oct 12, 2022

We can always add lint-staged later. It may be a bit tricky because it’s at the root of the project and stylelint isn’t installed there. So I’d say if we are good with the other changes that we should merge this and work on lint-staged separately.

@nzakas
Copy link
Member

nzakas commented Oct 25, 2022

@eslint/website-team please review this PR, merge in any suggested changes, and approve so we can move forward. Thanks!

@harish-sethuraman
Copy link
Member

I'm not able to commit suggested change as I don't have write access to this repo 😅 . Apart from the only lint:fix:scss suggestion still to be committed all other changes looks good.

docs/package.json Outdated Show resolved Hide resolved
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 26, 2022
@mdjermanovic
Copy link
Member

@harish-sethuraman
Copy link
Member

I guess some font is missing in the deploy preview?

@nschonni
Copy link
Contributor Author

It seems like the computed CSS is different:
PR:

"Inter", -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Helvetica, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"

Live:

"Mono Punctuators", "Space Mono", monospace, Consolas, Monaco, "Andale Mono", "Ubuntu Mono", monospace

docs/.stylelintrc.json Outdated Show resolved Hide resolved
@nschonni
Copy link
Contributor Author

Looks like the missing comma in the font variable was the issue. Preview is looking correct again
image

Should I rebase to see if there is anything in the changes to _include/layouts that might be different from when this was opened?

@nschonni
Copy link
Contributor Author

Rebased, but left the code review suggestion commits. I can merge those into the particular commit to keep it clean, but I wasn't sure if the plan was for a squash/merge in the end

@mdjermanovic
Copy link
Member

Rebased, but left the code review suggestion commits. I can merge those into the particular commit to keep it clean, but I wasn't sure if the plan was for a squash/merge in the end

It's okay to leave all commits, we always squash-merge in the end.

@nschonni
Copy link
Contributor Author

One other one I noticed when I was trying to figure out the Font thing, was that there is some CSS in <style> tags in the layout files. Do you want me to add this https://www.npmjs.com/package/stylelint-config-html in this PR, or keep that for a separate thing? It only seems to adjust the intend on the font declaration in base.html when I ran it

@mdjermanovic
Copy link
Member

Do you want me to add this https://www.npmjs.com/package/stylelint-config-html in this PR, or keep that for a separate thing?

It might be better to evaluate this separately. There are already a lot of changes in this PR, and our .html files are not raw HTML so we should doublecheck if linting actually works.

Copy link
Member

@harish-sethuraman harish-sethuraman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 8a15968 into eslint:main Oct 27, 2022
19 checks passed
@nschonni nschonni deleted the docs-stylelint branch October 27, 2022 13:25
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Nov 10, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.26.0` -> `8.27.0`](https://renovatebot.com/diffs/npm/eslint/8.26.0/8.27.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.27.0`](https://github.com/eslint/eslint/releases/tag/v8.27.0)

[Compare Source](eslint/eslint@v8.26.0...v8.27.0)

#### Features

-   [`f14587c`](eslint/eslint@f14587c) feat: new `no-new-native-nonconstructor` rule ([#&#8203;16368](eslint/eslint#16368)) (Sosuke Suzuki)
-   [`978799b`](eslint/eslint@978799b) feat: add new rule `no-empty-static-block` ([#&#8203;16325](eslint/eslint#16325)) (Sosuke Suzuki)
-   [`69216ee`](eslint/eslint@69216ee) feat: no-empty suggest to add comment in empty BlockStatement ([#&#8203;16470](eslint/eslint#16470)) (Nitin Kumar)
-   [`319f0a5`](eslint/eslint@319f0a5) feat: use `context.languageOptions.ecmaVersion` in core rules ([#&#8203;16458](eslint/eslint#16458)) (Milos Djermanovic)

#### Bug Fixes

-   [`c3ce521`](eslint/eslint@c3ce521) fix: Ensure unmatched glob patterns throw an error ([#&#8203;16462](eslint/eslint#16462)) (Nicholas C. Zakas)
-   [`886a038`](eslint/eslint@886a038) fix: handle files with unspecified path in `getRulesMetaForResults` ([#&#8203;16437](eslint/eslint#16437)) (Francesco Trotta)

#### Documentation

-   [`ce93b42`](eslint/eslint@ce93b42) docs: Stylelint property-no-unknown ([#&#8203;16497](eslint/eslint#16497)) (Nick Schonning)
-   [`d2cecb4`](eslint/eslint@d2cecb4) docs: Stylelint declaration-block-no-shorthand-property-overrides ([#&#8203;16498](eslint/eslint#16498)) (Nick Schonning)
-   [`0a92805`](eslint/eslint@0a92805) docs: stylelint color-hex-case ([#&#8203;16496](eslint/eslint#16496)) (Nick Schonning)
-   [`74a5af4`](eslint/eslint@74a5af4) docs: fix stylelint error ([#&#8203;16491](eslint/eslint#16491)) (Milos Djermanovic)
-   [`324db1a`](eslint/eslint@324db1a) docs: explicit stylelint color related rules ([#&#8203;16465](eslint/eslint#16465)) (Nick Schonning)
-   [`94dc4f1`](eslint/eslint@94dc4f1) docs: use Stylelint for HTML files ([#&#8203;16468](eslint/eslint#16468)) (Nick Schonning)
-   [`cc6128d`](eslint/eslint@cc6128d) docs: enable stylelint declaration-block-no-duplicate-properties ([#&#8203;16466](eslint/eslint#16466)) (Nick Schonning)
-   [`d03a8bf`](eslint/eslint@d03a8bf) docs: Add heading to justification explanation ([#&#8203;16430](eslint/eslint#16430)) (Maritaria)
-   [`8a15968`](eslint/eslint@8a15968) docs: add Stylelint configuration and cleanup ([#&#8203;16379](eslint/eslint#16379)) (Nick Schonning)
-   [`9b0a469`](eslint/eslint@9b0a469) docs: note commit messages don't support scope ([#&#8203;16435](eslint/eslint#16435)) (Andy Edwards)
-   [`1581405`](eslint/eslint@1581405) docs: improve context.getScope() docs ([#&#8203;16417](eslint/eslint#16417)) (Ben Perlmutter)
-   [`b797149`](eslint/eslint@b797149) docs: update formatters template ([#&#8203;16454](eslint/eslint#16454)) (Milos Djermanovic)
-   [`5ac4de9`](eslint/eslint@5ac4de9) docs: fix link to formatters on the Core Concepts page ([#&#8203;16455](eslint/eslint#16455)) (Vladislav)
-   [`33313ef`](eslint/eslint@33313ef) docs: core-concepts: fix link to semi rule ([#&#8203;16453](eslint/eslint#16453)) (coderaiser)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xOS4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjEuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1628
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion build This change relates to ESLint's build process contributor pool documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants