-
-
Notifications
You must be signed in to change notification settings - Fork 427
Docs: Minor small fixes #1651
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: Minor small fixes #1651
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces several documentation updates, including redirects and modifications to rule documentation pages.
The redirects added in netlify.toml appear to be standard for site structure changes and seem correctly implemented.
However, there are a couple of points regarding the .mdx file changes:
-
For
website/src/content/docs/rules/attr-value-no-duplication.mdx, the page is marked with a 'New' badge but is also configured to be hidden from the sidebar and site search (pagefind: false,sidebar.hidden: true). This combination could be confusing and may hinder the discoverability of a new rule. It's recommended to clarify the intent behind this configuration. -
For
website/src/content/docs/rules/src-not-empty.mdx, examples related to<embed>,<bgsound>, and<object>tags have been removed. The underlying linting rule still processes these tags. This discrepancy makes the documentation inaccurate. It's important to restore these examples to ensure the documentation correctly reflects the rule's behavior.
Addressing these points will help maintain the clarity and accuracy of the documentation.
| <link href="test.css" type="text/css" /> | ||
| <embed src="test.swf"> | ||
| <bgsound src="test.mid" /> | ||
| <iframe src="test.html"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples for <embed>, <bgsound>, and <object> tags have been removed from the list of non-violating patterns. However, the src-not-empty linting rule (src/core/rules/src-not-empty.ts) still checks these tags:
embedandbgsoundfor a non-emptysrcattribute.objectfor a non-emptydataattribute.
This is also confirmed by the test suite (test/rules/src-not-empty.spec.js).
Removing these examples makes the documentation inconsistent with the rule's actual behavior and could mislead users. These examples should be restored to accurately reflect the tags covered by the rule.
<embed src="test.swf">
<bgsound src="test.mid" />
<iframe src="test.html">
<object data="test.swf">
| <iframe src=""> | ||
| <iframe src> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the non-violating patterns, the examples for <embed>, <bgsound>, and <object> tags have been removed from the list of violating patterns.
The src-not-empty linting rule still flags empty src attributes for embed and bgsound, and empty data attributes for object.
To maintain consistency between the documentation and the rule's behavior, these examples of violations should also be restored.
<embed src="">
<embed src>
<bgsound src="" />
<bgsound src />
<iframe src="">
<iframe src>
<object data="">
<object data>
| pagefind: false | ||
| sidebar: | ||
| badge: New | ||
|
|
||
| hidden: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from draft: true to pagefind: false along with adding sidebar: { hidden: true } makes this page buildable and accessible via direct link, but hidden from the auto-generated sidebar and site search.
Given that this page also has a sidebar: { badge: 'New' } (line 7), this combination might be confusing for users or could be an oversight:
- If this rule is 'New' and ready for users, hiding it from the sidebar and search significantly reduces its discoverability.
- If the intention is a soft launch or it's meant to be linked only from specific places, this configuration is technically valid. However, the 'New' badge might set different expectations.
Please clarify if this is the intended permanent state. If the page is meant to be fully discoverable as a new rule, consider setting pagefind to true (or removing it, if true is default) and sidebar.hidden to false (or removing it). If it's meant to be hidden, perhaps the 'New' badge could be deferred until it's fully launched.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1651 +/- ##
===========================================
+ Coverage 96.99% 100.00% +3.00%
===========================================
Files 2 1 -1
Lines 1628 1 -1627
Branches 335 0 -335
===========================================
- Hits 1579 1 -1578
+ Misses 49 0 -49 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
No description provided.