Fix the table with examples in Wireguard component #5395
Conversation
✅ Deploy Preview for esphome ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Huh... HTML is allowed in Hugo but is forbidden in linter... What's the use case?.. |
WalkthroughReformatted the "Static routes and outgoing connections" subsection in content/components/wireguard.md from an ASCII table to an HTML table, preserving the original data and text without altering semantics or behavior. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
content/components/wireguard.md (2)
183-184: Optional: use semantic lists inside table cells for readability/accessibility.Replace manual “- …
” withlists.
- …
Example diff (apply analogously to each cell):
- <td>- 172.16.0.0/24<br/>- 192.168.0.0/24<br/>- <em>any other</em></td> + <td> + <ul> + <li>172.16.0.0/24</li> + <li>192.168.0.0/24</li> + <li><em>any other</em></li> + </ul> + </td>Also applies to: 196-197, 206-207
161-168: Optional: add a caption for context (better a11y).A caption clarifies purpose for screen readers and improves usability.
Apply this diff:
-<table> +<table> + <caption>Examples of netmask/allowed IP combinations and resulting outgoing connections</caption>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
content/components/wireguard.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
content/components/wireguard.md
🪛 GitHub Actions: Lint
content/components/wireguard.md
[error] 161-161: markdownlint MD033/no-inline-html Inline HTML [Element: table].
🔇 Additional comments (2)
content/components/wireguard.md (2)
161-210: Good fix: HTML table restores rowspan layout correctly.The conversion to an HTML table accurately preserves the intended rowspan semantics and fixes the previously broken Markdown rendering. Use of thead/tbody is appropriate.
161-210: Fix pipeline: silence markdownlint MD033 for this HTML block.CI fails with MD033 (no-inline-html). Wrap the table with markdownlint disable/enable comments.
+<!-- markdownlint-disable MD033 --> <table> <thead> <tr> <th>address</th> <th>netmask</th> <th>allowed ips</th> <th>working outgoing connections</th> </tr> </thead> <tbody> ... </tbody> </table> +<!-- markdownlint-enable MD033 -->Re-run the docs lint workflow to confirm MD033 is resolved.
|
@clydebarrow could you check this as the author of the "breaking change"? |
|
No, we don't want embedded HTML. Since rowspan is not available in markdown, just add blank cells to simulate spanned rows. It's not ideal, but it gets the job done. Which is what happened in the conversion, just looks like some text got lost which should be restored. Combine the two cells for "omitted" and "176.16..." |
Not only that. What to do with yaml lists ("allowed ips" column)? We need Not just I believe, |
|
You can insert line breaks inside table cells with |
Chupaka
force-pushed
the
fix_wireguard_table
branch
2 times, most recently
from
453e8c1 to
4c4e771
Compare
|
I updated the PR, but neither Also, please pick one - probably blank cells are less informative: 
VS
|
I added a break shortcode. There is also the possibility to add a table render hook that could implement row and column spans, but that's a bit more work. Re the two alternatives, probably repeating the same text is better than a blank cell. |
Chupaka
force-pushed
the
fix_wireguard_table
branch
from
90240e8 to
6aeef1c
Compare
Chupaka
force-pushed
the
fix_wireguard_table
branch
from
6aeef1c to
4c87eea
Compare
I like this better too. So, looks like we're done, thanks for your assistance! |
|
Not a big deal in this case, but force-pushing to a PR after review has started is generally not a Good Idea - it means the reviewer has to start over again since the history is lost. All commits get squashed when merged so there is no problem with having lots of small commits. |
2 participants
Description:
Previously the table was a bit complex:
Recent conversion to Markdown broke the table: no newlines, even some text was lost:
As there's no possibility to use
rowspanin Markdown, converting the table to HTML manually:Related issue (if applicable): fixes
Checklist:
I am merging into
nextbecause this is new documentation that has a matching pull-request in esphome as linked above.or
I am merging into
currentbecause this is a fix, change and/or adjustment in the current documentation and is not for a new component or feature.Link added in
/components/index.rstwhen creating new documents for new components or cookbook.