Skip to content

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

Merged
merged 1 commit into from
Jun 11, 2025
Merged

Docs: Minor small fixes #1651

merged 1 commit into from
Jun 11, 2025

Conversation

coliff
Copy link
Member

@coliff coliff commented Jun 11, 2025

No description provided.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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:

  1. 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.

  2. 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">
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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 and bgsound for a non-empty src attribute.
  • object for a non-empty data 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">

Comment on lines 37 to 38
<iframe src="">
<iframe src>
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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>

Comment on lines +5 to +8
pagefind: false
sidebar:
badge: New

hidden: true
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

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.

Copy link

codecov bot commented Jun 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (95e6a13) to head (940472e).
Report is 72 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7de144...940472e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coliff coliff merged commit 4629b76 into main Jun 11, 2025
19 of 20 checks passed
@coliff coliff deleted the dev/coliff/minor-small-docs-fixes branch June 11, 2025 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant