Skip to content

chore(website): automate the addition of rule attributes to the website #5085

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

Merged
merged 3 commits into from
May 30, 2022

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented May 27, 2022

PR Checklist

Overview

@JoshuaKGoldberg and I hacked on this during the stream today.
This adds a remark plugin to our docusaurus config which automatically generates the "Attributes" section on each of the rule doc pages.
This is great because it removes the need for a test and manual maintenance!

All of the rule docs are markdown files that are automatically loaded into the website via mdx. Because we don't have a component that wraps these, Josh and I thought that using a remark plugin seemed like the best way to make this happen.

If someone with docusaurus experience knows a better way - please let me know! This worked, but it seems kind-of hacky?

image

@bradzacher bradzacher added the package: website Issues related to the @typescript-eslint website label May 27, 2022
@nx-cloud
Copy link

nx-cloud bot commented May 27, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8b5862d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 47 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@netlify
Copy link

netlify bot commented May 27, 2022

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 8b5862d
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/62950ff4bc4b9d0009c4e98d
😎 Deploy Preview https://deploy-preview-5085--typescript-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.

@Josh-Cena
Copy link
Member

Josh-Cena commented May 27, 2022

#4976 cries "merge conflict"! Nvm, I think it touches a different part of the metadata, but that can be automated eventually as well

@bradzacher bradzacher force-pushed the automate-rule-attributes branch from b6d1221 to 1bc2c80 Compare May 27, 2022 05:19
Comment on lines +112 to +115
/* indent the nested checklist for the rule doc attributes */
ul.contains-task-list > li > ul.contains-task-list {
padding-left: 24px;
}
Copy link
Member

Choose a reason for hiding this comment

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

This has been fixed by facebook/docusaurus#7438

Copy link
Member

Choose a reason for hiding this comment

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

We can rebase and remove this now.

@bradzacher
Copy link
Member Author

bradzacher commented May 28, 2022

I kind of hate having this information right there in the docs cos it feels long and feels like it gets in the way.
I disliked having it at the end as well cos it felt like it was easy to miss.

Two ideas:

  1. put it in the right-hand sidebar (where the TOC is).
    • We never have many headings so adding it there seems like a decent use of space
    • However this wouldn't work on mobile, I think?
  2. maybe use a table or similar to lay out out horizontally like
Configs Fixers Other
☒ ✅ Recommended ☐ 🔧 Automated Fixer ☒ 💭 Requires type information
☐ 🔒 Strict ☒ 🛠 Suggestion Fixer

@Josh-Cena
Copy link
Member

Josh-Cena commented May 28, 2022

Mobile would not be a big problem: we would be customizing the React component to inject this information anyway, we can well put it in the dropdown where the TOC lives:

image

Or we can also have a floating button that expands to the metadata table.

const requiresTypeInfo = rule.meta.docs?.requiresTypeChecking === true;

const parent = root as mdast.Parent;
/*
Copy link
Member

Choose a reason for hiding this comment

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

I found the package that does this for us:

import { fromMarkdown } from "mdast-util-from-markdown";

fromMarkdown(`## Attributes
  ...
`);

Especially since we're going to automate more of this page, I feel like that's a good approach?

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Woop! 🚀

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #5085 (8b5862d) into main (e0db364) will increase coverage by 2.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #5085      +/-   ##
==========================================
+ Coverage   91.70%   93.83%   +2.12%     
==========================================
  Files         362      286      -76     
  Lines       12181     9838    -2343     
  Branches     3530     2939     -591     
==========================================
- Hits        11171     9231    -1940     
+ Misses        661      328     -333     
+ Partials      349      279      -70     
Flag Coverage Δ
unittest 93.83% <ø> (+2.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/utils/src/eslint-utils/nullThrows.ts
...ckages/utils/src/eslint-utils/getParserServices.ts
...ckages/utils/src/ts-eslint-scope/PatternVisitor.ts
packages/type-utils/src/getTokenAtPosition.ts
packages/utils/src/ast-utils/helpers.ts
packages/utils/src/ast-utils/misc.ts
packages/utils/src/ts-eslint/ParserOptions.ts
packages/utils/src/ts-eslint-scope/Definition.ts
...ges/utils/src/ast-utils/eslint-utils/predicates.ts
packages/type-utils/src/predicates.ts
... and 66 more

@bradzacher bradzacher mentioned this pull request May 30, 2022
3 tasks
@bradzacher bradzacher merged commit 45d8ad3 into main May 30, 2022
@bradzacher bradzacher deleted the automate-rule-attributes branch May 30, 2022 20:49
@bradzacher
Copy link
Member Author

I'll punt on the parsing and the structure for now so that others can continue their work on the website.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: website Issues related to the @typescript-eslint website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants