-
-
Notifications
You must be signed in to change notification settings - Fork 414
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.
@@ -23,10 +23,7 @@ Level: <Badge text="Error" variant="danger" /> | |||
<img src="test.png" /> | |||
<script src="test.js"></script> | |||
<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:
embed
andbgsound
for a non-emptysrc
attribute.object
for a non-emptydata
attribute.
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.