Introduce markdown linting #2726
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
size-limit report 📦
|
khiga8
force-pushed
the
kh-add-markdown-lint
branch
from
e8edee8 to
f49aeec
Compare
|
There are 80 violations. I could either try to address in this PR, but if they are too difficult to address in one go, I'll turn off the rule completely with a note that we want to turn on eventually... Here are the issues that need to be resolved: |
|
I think we could configure this rule to be |
|
Can I get a first-pass review from @primer/react-reviewers on these configs? |
There was a problem hiding this comment.
👋🏼 @khiga8 Thank you so much for this PR 🙏🏼 I left a couple of comments, let us know what you think!
| @@ -45,6 +45,7 @@ | |||
| "install:docs": "(cd docs && npm install --legacy-peer-deps)", | |||
| "lint": "eslint '**/*.{js,ts,tsx,md,mdx}' --max-warnings=0", | |||
There was a problem hiding this comment.
We have a set of rules for the md and mdx files already on the eslint config. I am not sure if there would be any conflict between them? Or what do you suggest for us to do here?
There was a problem hiding this comment.
oooh interesting! I don't think eslint runs the same rulesets as markdownlint. do you know what ESLint rules are running on markdown?
…nly increment by one level at a time
khiga8
commented
Jan 10, 2023
| // Rules we want to turn on but currently have too many violations | ||
| const rulesToEnforce = { | ||
| 'fenced-code-language': false, | ||
| 'no-duplicate-header': false, // Fix https://github.com/primer/doctocat/issues/527, then set this rule to `siblings_only: true` |
There was a problem hiding this comment.
@primer/react-reviewers FYI,no-duplicate-header should be turned on because doctocat messes up the anchor links when headings have the same text. See linked issue. I keep it off for now because there's too many address.
Ideally, the issue is fixed in doctocat so we don't have to set this rule to be strict.
khiga8
commented
Jan 10, 2023
| @@ -38,7 +39,7 @@ On the other hand, if we want consumers to have more control over children, a co | |||
|
|
|||
| With React, `children` is the out-of-the-box way for putting [phrasing content](https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_categories#phrasing_content) inside your component. By using `children` instead of our own custom prop, we can make the API “predictable” for its consumers. | |||
|
|
|||
| <img width="373" alt="image" src="https://user-images.githubusercontent.com/1863771/144945223-70c4c800-5827-4985-9f18-0ab416eba058.png"> | |||
| <img width="373" alt="image" src="https://user-images.githubusercontent.com/1863771/144945223-70c4c800-5827-4985-9f18-0ab416eba058.png"> <!-- markdownlint-disable-line no-default-alt-text --> | |||
There was a problem hiding this comment.
@primer/react-reviewers This is a TODO! Feel free to suggest something in this PR to fix this!
khiga8
commented
Jan 10, 2023
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| Consider a React component that renders a list of items. Here are two possible APIs that component might expose, both achieving an equivalent result. | |||
|
|
|||
| ### A: Contents passed as React children | |||
| ### A: Contents passed as React children <!-- markdownlint-disable-line heading-increment --> | |||
There was a problem hiding this comment.
@primer/react-reviewers This is a TODO! I'm not sure what the heading structure should be. Feel free to suggest something in this PR to fix this!
khiga8
commented
Jan 10, 2023
| ``` | ||
|
|
||
| [Show full code example →](https://github.com/primer/react/pull/2019/files?diff=split&w=0) | ||
| ```tsx |
There was a problem hiding this comment.
Errr I think prettier ran for these files and updated it, but not sure. @primer/react-reviewers is this expected?
khiga8
commented
Jan 10, 2023
Comment on lines
+9
to
+22
| const rulesToNotEnforce = { | ||
| 'line-length': false, | ||
| 'blanks-around-headings': false, | ||
| 'blanks-around-lists': false, | ||
| 'no-trailing-spaces': false, | ||
| 'no-multiple-blanks': false, | ||
| 'no-trailing-punctuation': false, | ||
| 'single-trailing-newline': false, | ||
| 'ul-indent': false, | ||
| 'no-hard-tabs': false, | ||
| 'first-line-heading': false, | ||
| 'no-space-in-emphasis': false, | ||
| 'blanks-around-fences': false, | ||
| } |
There was a problem hiding this comment.
These are stylistic rules that I don't think Primer team cares to enforce for now, but not sure. Feel free to configure this to your preference!!!
|
@primer/react-reviewers This is ready for another review. Please review thoroughly as this will impact developer workflow! For the existing violations listed in #2726 (comment), I addressed some of them. You can go through them by commit. There were two that I disabled inline, which are open to your fix suggestions: Two of the rules, there were too many of to address immediately so I turned them off completely for now. |
joshblack
approved these changes
Jan 11, 2023
.github/workflows/ci.yml
Outdated
Comment on lines
43
to
44
| - name: Lint | ||
| run: npm run markdownlint |
There was a problem hiding this comment.
Could we add this to the lint job as a step after npm run lint? Would be great to keep it in the same job for linting, something like:
- name: Lint JavaScript
run: npm run lint
- name: Lint markdown
run: npm run markdownlint
There was a problem hiding this comment.
Sure, I can do that! I got feedback in the primer/design repo that it's more clear when there are separate linting jobs for error clarity, but we can keep it together if you prefer. I don't have a strong opinion.
|
note to self to update the |
|
Hi @joshblack! I addressed your review suggestions in d1f87d2. I additionally updated the |
|
Thanks for doing this @khiga8! 🙌 |
joshblack
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: https://github.com/github/accessibility/issues/2550
This PR:
devcontainerso that issues are flagged in the editor.eslintandmarkdownlint.