feat: documentation site redesign #5854
Conversation
- Refactor main.css with improved layout and responsive styles - Update SVG icons with consistent styling - Simplify sidebar and menu JavaScript - Clean up baseof.html and index layout - Minor content adjustments to index and getting started guide
- Implement consistent grid layout for component cards - Add 60/40 split between image and text areas - Improve text wrapping with text-wrap: pretty - Add background color differentiation for text area - Increase card size and minimum dimensions - Remove link styling from component cards
- Refine color palette with improved contrast - Add new design tokens (heading-color, text-secondary, surface-color) - Update navigation height and styling - Improve border colors and spacing - Add theme transition disabling utility
- Improve blockquote and alert styling - Update heading markup rendering - Refine project and single page layouts - Update shortcode templates for consistency
- Update existing icon SVGs for consistency - Add new icons (sun, moon, check, close, etc.) - Standardize icon formatting
✅ Deploy Preview for esphome ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughNormalized copy and icon identifiers across content, replaced emoji alert icons with SVGs, introduced theme-aware CSS variables and typography, reworked sidebar/TOC rendering, enhanced client JS for menu/TOC/theme (scroll‑spy with pause/resume), adjusted templates/shortcodes, and added dev-mode asset cache‑busting. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Browser
participant MenuLogic as Menu/Overlay Logic
participant TOC as TOC Link Handler
participant Headings as Page Headings
participant Observer as IntersectionObserver
Note over MenuLogic,Observer: Menu/TOC/scroll‑spy & theme interactions
User->>Browser: Click hamburger / open menu
Browser->>MenuLogic: openMenu() -> show overlay, set menu state
User->>TOC: Click TOC link
TOC->>Observer: pause scroll‑spy
TOC->>Headings: smooth scroll to target (programmatic)
Headings-->>TOC: reached target
TOC->>Observer: resume scroll‑spy after delay
Observer->>TOC: highlight active TOC entry based on topmost visible heading
User->>Browser: Select theme option
Browser->>MenuLogic: setTheme() -> temporarily disable transitions, persist choice, update data-theme
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
themes/esphome-theme/layouts/partials/sidebar.html (1)
20-20: RemovesafeHTMLfrom heading title output to prevent XSS vulnerabilities.The
.Titlevalues come from Markdown heading content (user-editable and untrusted). ThereplaceREfilter only removes trailing whitespace and does not sanitize HTML tags or script content. UsingsafeHTMLthen bypasses template escaping, allowing embedded HTML or script payloads in headings to execute unescaped in the DOM. This is especially risky given theunsafe: truesetting inhugo.yaml's goldmark renderer config, which permits raw HTML in Markdown.Remove the
| safeHTMLpipe to use Hugo's default HTML escaping, which safely handles heading titles. If HTML in headings is intentional, sanitize the content server-side before rendering or override the heading render hook to explicitly escape dangerous content.themes/esphome-theme/assets/css/main.css (2)
1732-1741: Hardcoded color in figcaption should use CSS variable.Line 1735 uses hardcoded
color: #666which won't adapt to dark mode. This should use--text-secondaryfor theme consistency.Suggested fix
figcaption { margin-top: 0.75em; font-size: 0.9em; - color: #666; + color: var(--text-secondary); font-style: italic; line-height: 1.4; max-width: 80%; margin-left: auto; margin-right: auto; }
7-65: Address WCAG AA color contrast compliance issue.The design token system is well-structured with good semantic naming. However, verification found that
--feature-card-text-secondary: rgba(0, 0, 0, 0.55)on--feature-card-bg: #ffffffproduces a contrast ratio of only 3.36:1, which fails WCAG AA requirement of 4.5:1 for normal text. Adjust the secondary text color to meet accessibility compliance.
🧹 Nitpick comments (8)
themes/esphome-theme/layouts/_default/_markup/render-blockquote-alert.html (1)
11-11: Consider adding error handling for missing alert types or icon files.The current implementation will cause a build failure if:
- An alert type is used that's not defined in the
$iconsdictionary- A referenced SVG file doesn't exist
While build-time failures are acceptable for a documentation site, consider adding a fallback icon or validation to provide clearer error messages for maintainers.
🔎 Optional: Add a default fallback icon
- <span class="alert-icon">{{ os.ReadFile (printf "static/images/icons/%s.svg" (index $icons .AlertType)) | safeHTML }}</span> + <span class="alert-icon">{{ os.ReadFile (printf "static/images/icons/%s.svg" (index $icons .AlertType | default "circle-info")) | safeHTML }}</span>This ensures unknown alert types fall back to the
circle-infoicon rather than breaking the build.themes/esphome-theme/layouts/index.html (1)
6-10: Consider addingrel="noopener noreferrer"to the external link.The Device database link points to an external domain. While
devices.esphome.iois a trusted ESPHome property, adding security attributes is a defensive best practice for external links.🔎 Suggested change
<div class="cta-buttons"> <a href="{{ "guides/getting_started_hassio/" | relURL }}" class="btn btn-primary">Get started</a> <a href="{{ "components/" | relURL }}" class="btn btn-secondary">Browse components</a> - <a href="https://devices.esphome.io/" class="btn btn-secondary">Device database</a> + <a href="https://devices.esphome.io/" class="btn btn-secondary" rel="noopener noreferrer">Device database</a> </div>themes/esphome-theme/assets/js/main.js (2)
292-296: Add an initial value toreduce()to prevent potential runtime error.If
visibleEntriesis filtered to a non-empty array but somehow becomes empty during the reduce operation due to a race condition or browser quirk, callingreduce()without an initial value on an empty array throws aTypeError.🔎 Suggested fix
if (visibleEntries.length > 0) { // Get the topmost visible heading - const topEntry = visibleEntries.reduce((top, entry) => { - return entry.boundingClientRect.top < top.boundingClientRect.top ? entry : top; - }); + const topEntry = visibleEntries.reduce((top, entry) => { + return entry.boundingClientRect.top < top.boundingClientRect.top ? entry : top; + }, visibleEntries[0]);
338-342: Consider extracting the magic number80into a named constant.The offset
80appears to correspond to the navigation bar height. If this value changes (e.g., via CSS updates), it could cause misalignment. Consider deriving it from the actual nav height or a shared constant.🔎 Suggested approach
+ const navOffset = document.getElementById('nav-container')?.offsetHeight || 80; window.scrollTo({ - top: targetElement.offsetTop - 80, + top: targetElement.offsetTop - navOffset, behavior: 'smooth' });themes/esphome-theme/assets/css/main.css (4)
919-922: Consider accessibility ofdisplay: contents.Using
display: contents(Line 921) can have accessibility implications in some older browser versions where it removes elements from the accessibility tree. Since this is applied to icon containers within a button that has proper semantics, the impact should be minimal, but worth noting.Alternative approach using visibility
[data-theme="dark"] .moon-icon, [data-theme="light"] .sun-icon { - display: contents; + display: block; } + +[data-theme="dark"] .sun-icon, +[data-theme="light"] .moon-icon { + display: none; +}
1272-1284: Consider using CSS variables for button colors.The primary button uses hardcoded color values (
#00a8e0,#fff) instead of CSS variables. While functional, using variables like--btn-primary-color(which exists) and a new--btn-primary-textwould improve maintainability and theme consistency.Suggested refactor using variables
.btn-primary { - background-color: #00a8e0; - color: #fff; + background-color: var(--btn-primary-color); + color: var(--btn-text-color-inverse, #fff); }Add to
:root:--btn-text-color-inverse: #fff;
1058-1084: Consider reducing!importantusage in dark mode overrides.Several dark mode rules use
!important(e.g., Lines 1059, 1071-1073, 1078). While this ensures overrides work, it can make future maintenance harder. Since CSS variables already handle most theming, consider whether these!importantdeclarations are necessary or if they're compensating for specificity issues elsewhere.
1604-1609: Duplicate.cta-buttonsrule block.This
.cta-buttonsblock duplicates the one at Lines 1239-1244. While CSS allows multiple blocks that get merged, consolidating them would improve maintainability.Consolidated rule
.cta-buttons { display: flex; flex-wrap: wrap; flex-flow: row wrap; justify-content: flex-start; gap: var(--grid-gap-sm); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (49)
static/images/icons/account-tie.svgis excluded by!**/*.svgstatic/images/icons/bolt.svgis excluded by!**/*.svgstatic/images/icons/briefcase.svgis excluded by!**/*.svgstatic/images/icons/check.svgis excluded by!**/*.svgstatic/images/icons/chip.svgis excluded by!**/*.svgstatic/images/icons/circle-exclamation.svgis excluded by!**/*.svgstatic/images/icons/circle-info.svgis excluded by!**/*.svgstatic/images/icons/close.svgis excluded by!**/*.svgstatic/images/icons/cloud-download.svgis excluded by!**/*.svgstatic/images/icons/code-block-tags.svgis excluded by!**/*.svgstatic/images/icons/code.svgis excluded by!**/*.svgstatic/images/icons/comments.svgis excluded by!**/*.svgstatic/images/icons/compass.svgis excluded by!**/*.svgstatic/images/icons/computer.svgis excluded by!**/*.svgstatic/images/icons/database.svgis excluded by!**/*.svgstatic/images/icons/desktop-tower-monitor.svgis excluded by!**/*.svgstatic/images/icons/discord.svgis excluded by!**/*.svgstatic/images/icons/edit.svgis excluded by!**/*.svgstatic/images/icons/espressif.svgis excluded by!**/*.svgstatic/images/icons/github.svgis excluded by!**/*.svgstatic/images/icons/head-cog.svgis excluded by!**/*.svgstatic/images/icons/history.svgis excluded by!**/*.svgstatic/images/icons/home-automation.svgis excluded by!**/*.svgstatic/images/icons/home.svgis excluded by!**/*.svgstatic/images/icons/industry.svgis excluded by!**/*.svgstatic/images/icons/kitchen-set.svgis excluded by!**/*.svgstatic/images/icons/lightbulb.svgis excluded by!**/*.svgstatic/images/icons/link.svgis excluded by!**/*.svgstatic/images/icons/list.svgis excluded by!**/*.svgstatic/images/icons/magic.svgis excluded by!**/*.svgstatic/images/icons/memory.svgis excluded by!**/*.svgstatic/images/icons/menu.svgis excluded by!**/*.svgstatic/images/icons/microchip.svgis excluded by!**/*.svgstatic/images/icons/monitor.svgis excluded by!**/*.svgstatic/images/icons/moon.svgis excluded by!**/*.svgstatic/images/icons/ohf-logo-on-dark.svgis excluded by!**/*.svgstatic/images/icons/ohf-logo-on-light.svgis excluded by!**/*.svgstatic/images/icons/print.svgis excluded by!**/*.svgstatic/images/icons/puzzle-piece.svgis excluded by!**/*.svgstatic/images/icons/raspberry-pi.svgis excluded by!**/*.svgstatic/images/icons/search.svgis excluded by!**/*.svgstatic/images/icons/shape-plus.svgis excluded by!**/*.svgstatic/images/icons/shield-alt.svgis excluded by!**/*.svgstatic/images/icons/sun.svgis excluded by!**/*.svgstatic/images/icons/terminal.svgis excluded by!**/*.svgstatic/images/icons/triangle-exclamation.svgis excluded by!**/*.svgstatic/images/icons/user-cog.svgis excluded by!**/*.svgstatic/images/icons/wifi-strength-off.svgis excluded by!**/*.svgstatic/images/icons/wifi.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
content/_index.mdcontent/components/_index.mdcontent/guides/getting_started_hassio.mdhugo.yamlthemes/esphome-theme/assets/css/main.cssthemes/esphome-theme/assets/js/main.jsthemes/esphome-theme/assets/js/menu.jsthemes/esphome-theme/assets/js/search.jsthemes/esphome-theme/layouts/_default/_markup/render-blockquote-alert.htmlthemes/esphome-theme/layouts/_default/_markup/render-heading.htmlthemes/esphome-theme/layouts/_default/baseof.htmlthemes/esphome-theme/layouts/_default/list.htmlthemes/esphome-theme/layouts/_default/projects.htmlthemes/esphome-theme/layouts/_default/single.htmlthemes/esphome-theme/layouts/index.htmlthemes/esphome-theme/layouts/partials/asset.htmlthemes/esphome-theme/layouts/partials/sidebar.htmlthemes/esphome-theme/layouts/shortcodes/changelogs.htmlthemes/esphome-theme/layouts/shortcodes/collapse.htmlthemes/esphome-theme/layouts/shortcodes/getting-started-grid.htmlthemes/esphome-theme/layouts/shortcodes/imgtable.html
💤 Files with no reviewable changes (4)
- content/guides/getting_started_hassio.md
- themes/esphome-theme/layouts/_default/list.html
- themes/esphome-theme/layouts/_default/single.html
- themes/esphome-theme/layouts/_default/projects.html
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
content/components/_index.mdthemes/esphome-theme/assets/js/search.jsthemes/esphome-theme/layouts/shortcodes/collapse.htmlthemes/esphome-theme/layouts/partials/asset.htmlthemes/esphome-theme/layouts/_default/_markup/render-blockquote-alert.htmlthemes/esphome-theme/layouts/partials/sidebar.htmlthemes/esphome-theme/layouts/shortcodes/changelogs.htmlthemes/esphome-theme/layouts/shortcodes/imgtable.htmlcontent/_index.mdthemes/esphome-theme/layouts/index.htmlthemes/esphome-theme/layouts/shortcodes/getting-started-grid.htmlthemes/esphome-theme/layouts/_default/baseof.htmlthemes/esphome-theme/assets/css/main.csshugo.yamlthemes/esphome-theme/layouts/_default/_markup/render-heading.htmlthemes/esphome-theme/assets/js/menu.jsthemes/esphome-theme/assets/js/main.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: esphome/esphome-docs PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T00:13:13.459Z
Learning: Open pull requests against the correct base branch: use `current` for documentation-only revisions and `next` for changes tied to new features in esphome/esphome
Learnt from: jesserockz
Repo: esphome/esphome-docs PR: 4865
File: .github/workflows/needs-docs.yml:0-0
Timestamp: 2025-05-01T03:29:47.922Z
Learning: In the esphome-docs repository, the "current" label is automatically added by a bot to pull requests, making it a reliable indicator for the target branch.
📚 Learning: 2025-05-05T05:08:25.575Z
Learnt from: clydebarrow
Repo: esphome/esphome-docs PR: 0
File: :0-0
Timestamp: 2025-05-05T05:08:25.575Z
Learning: In ESPHome documentation, leading slashes in doc references are necessary when the source document is not at the root of the tree, as absolute paths to referenced documents are required in this context.
Applied to files:
content/components/_index.md
📚 Learning: 2025-05-01T03:29:47.922Z
Learnt from: jesserockz
Repo: esphome/esphome-docs PR: 4865
File: .github/workflows/needs-docs.yml:0-0
Timestamp: 2025-05-01T03:29:47.922Z
Learning: In the esphome-docs repository, the "current" label is automatically added by a bot to pull requests, making it a reliable indicator for the target branch.
Applied to files:
content/components/_index.md
📚 Learning: 2025-05-12T00:02:50.869Z
Learnt from: jesserockz
Repo: esphome/esphome-docs PR: 4901
File: changelog/2025.4.0.rst:124-126
Timestamp: 2025-05-12T00:02:50.869Z
Learning: In the ESPHome changelog structure, the "All changes" section should have the `:open:` attribute to be expanded by default, but the "Dependency Changes" section should NOT have this attribute as it should remain collapsed by default.
Applied to files:
themes/esphome-theme/layouts/shortcodes/collapse.htmlthemes/esphome-theme/layouts/partials/sidebar.htmlthemes/esphome-theme/layouts/shortcodes/changelogs.htmlcontent/_index.mdthemes/esphome-theme/layouts/index.htmlthemes/esphome-theme/layouts/_default/baseof.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (32)
themes/esphome-theme/layouts/partials/asset.html (1)
5-12: LGTM! Appropriate cache-busting for development.Adding timestamp-based cache-busting to development assets ensures browsers always fetch the latest CSS and JS files during local development. Production assets correctly remain unchanged with fingerprinting and integrity checks.
themes/esphome-theme/layouts/_default/_markup/render-blockquote-alert.html (1)
1-7: All referenced SVG icon files exist.The icon dictionary correctly maps to the following files in
static/images/icons/:
circle-exclamation.svgcircle-info.svglightbulb.svgtriangle-exclamation.svgNo issues identified.
themes/esphome-theme/layouts/partials/sidebar.html (4)
1-8: LGTM!The variable setup and container initialization are standard Hugo template patterns. The conditional
scroll-trapclass application is appropriate.
29-40: LGTM!The section list rendering logic is straightforward and handles edge cases appropriately (missing titles, empty pages).
43-53: LGTM!The GitHub edit link implementation follows standard Hugo patterns. The inline SVG rendering using
os.ReadFileis appropriate for static assets.
14-17: No bug: Empty TOC from missing Level 0 nested headings is handled safely by Hugo.The edge case where a Level 0 heading lacks nested
.Headingsis handled gracefully. If(index $headings 0).Headingsis nil,$headingsbecomes nil and therangeloop over it produces an empty TOC—not an error. This is correct behavior: Level 0 headings function as containers for actual content headings, so missing children should render nothing. Hugo'swithandrangeconstructs safely handle nil and empty values throughout the template.themes/esphome-theme/assets/js/search.js (1)
102-102: LGTM! Clean placeholder text.The shortened placeholder aligns with the broader UI text normalization in this PR.
themes/esphome-theme/layouts/shortcodes/changelogs.html (1)
41-41: LGTM! Improved accessibility.Adding visible summary text improves usability by clearly labeling the collapsible section.
themes/esphome-theme/layouts/shortcodes/getting-started-grid.html (1)
23-23: LGTM! Intentional button styling update.The class change from
btn-primarytobtn-secondaryaligns with the broader button system refinements in this PR.themes/esphome-theme/layouts/shortcodes/collapse.html (1)
23-23: LGTM! Improved accessibility.Providing a visible summary label makes the collapsible section clearer to users and improves keyboard navigation.
hugo.yaml (1)
30-86: LGTM! Consistent menu label normalization.The capitalization changes from Title Case to sentence case are applied consistently across all main menu items, aligning with the broader text normalization objectives of this PR.
themes/esphome-theme/layouts/_default/_markup/render-heading.html (1)
3-6: LGTM!The dual-icon approach with a container span is well-structured for the copy-link feedback feature. Good accessibility practices with
aria-hidden="true"and emptyaltattributes for decorative images.content/components/_index.md (1)
15-21: LGTM!The condensed TIP callout is a clear improvement over the verbose multi-paragraph block. It preserves the essential information (GitHub PR, Edit on GitHub, Discord) in a more scannable format that aligns with the PR's goal of streamlined documentation.
themes/esphome-theme/assets/js/main.js (2)
116-121: LGTM!Defensive fix for empty
<summary>elements ensures consistent UX across browsers. The "Show details" default text aligns with typical disclosure widget conventions.
139-141: LGTM!Simplified copy feedback using a CSS class is cleaner than manual animation triggers. The 1-second timeout provides appropriate visual feedback duration.
content/_index.md (3)
27-45: LGTM!Icon identifier updates and sentence-case normalization are consistent throughout. The changes align with the PR's broader styling standardization effort.
50-75: LGTM!Section heading and feature descriptions consistently follow sentence-case conventions, improving visual consistency across the site.
105-134: LGTM!Getting started section headings and button text normalized to sentence case. Changes are consistent with the rest of the content updates.
themes/esphome-theme/layouts/_default/baseof.html (3)
61-64: Verify font loading performance impact.External Google Fonts adds a network dependency. The
preconnecthints help, but consider self-hosting fonts for improved performance and privacy if this becomes a concern.
108-117: LGTM!The theme toggle implementation is well-structured with:
- Desktop toggle button with sun/moon icons
- Mobile-specific picker with explicit Light/Dark options
- Proper
aria-labelfor accessibilityGood separation of desktop and mobile UX patterns.
155-159: LGTM!Using separate light/dark logo images with CSS class-based visibility (
light-only/dark-only) is a clean approach for theme-aware branding without JavaScript.themes/esphome-theme/assets/js/menu.js (4)
11-19: LGTM!Proper state coordination - overlay is only hidden if the TOC isn't currently open, preventing UI glitches when closing one panel while another is active.
59-71: LGTM!The double
requestAnimationFrameis a well-known pattern to ensure CSS transitions are properly disabled during theme changes and re-enabled only after the browser has painted the new theme. This prevents flash-of-transition artifacts.
76-94: LGTM!Theme toggle handlers correctly use
stopPropagation()to prevent menu closure when clicking theme controls. The initial active state setup ensures the mobile picker reflects the current theme on page load.
105-110: LGTM!Overlay click handler properly closes both TOC and menu, ensuring consistent dismissal behavior regardless of which panel triggered the overlay.
themes/esphome-theme/assets/css/main.css (4)
2-5: LGTM - Theme transition disable utility.Good approach to prevent flash-of-unstyled-content during theme switches by disabling all transitions with
!important. The universal selector with descendant combinator ensures comprehensive coverage.
773-781: Good focus state implementation.The search input properly replaces the default outline with a visible custom focus indicator (border-color change and box-shadow). This maintains keyboard accessibility while matching the design system.
1847-1870: Clever clip-path animation for mobile menu.The use of
clip-path: inset()for the mobile menu reveal animation is smooth and performant. The negative bottom inset (-20px) on the active state correctly accounts for the box-shadow overflow. Thepointer-eventstoggle ensures proper interaction states.
1143-1152: Good use of pseudo-element for full-bleed sidebar background.The
::beforepseudo-element extending50vwto the left is a clean technique for creating a full-bleed background effect without affecting layout. Thepointer-events: nonecorrectly prevents interaction interference.themes/esphome-theme/layouts/shortcodes/imgtable.html (3)
51-59: LGTM - Component text wrapper addition.The new
<div class="component-text">wrapper (Lines 51-59) properly groups the component name and caption, aligning with the CSS styling defined inmain.css(.component-textat Lines 1643-1650). This improves the component card layout structure.
52-54: Good word-breaking technique for long component names.The regex-based insertion of zero-width space (
​) after non-alphabetic characters enables natural word-breaking for long technical names (e.g.,ESP32-S3-DevKitC-1can break at hyphens). ThesafeHTMLcall is correctly used since HTML entities are being inserted.
35-43: Caption concatenation logic is clear.The loop correctly handles the optional parameters:
dark-invertis captured as a class, while other non-empty values are treated as captions and concatenated with" & ". This matches the documented usage patterns in the header comment.
|
No meaningful opinion on the case change for 2nd word in titles & buttons, but if we are going to make that change, it should be added to the linter |
- Add blank lines after anchor shortcodes in getting_started_hassio.md - Fix italic formatting (use _ instead of *) - Remove Google Fonts links, fonts now self-hosted
- Add @font-face declarations for Instrument Sans and Figtree - Reorganize CSS variables with better grouping and comments - Add new design tokens (shadows, hover overlays) - Improve code organization and maintainability - Update syntax highlighting styles - Improve codeblock copy button implementation
- Add Instrument Sans and Figtree font files - Add content-copy icon SVG
woff2 font files were being incorrectly parsed as text, causing lint failures. Added binary_types list and skip them alongside image_types in content check loop.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
themes/esphome-theme/assets/js/main.js (1)
157-164: Remove dead code and consider consistent feedback duration.The
copy-feedbackelement no longer exists inrender-codeblock.html(lines 3-8 show only SVG icons now), making lines 157-160 dead code. Additionally, the timeout here is 2000ms vs 1000ms for.copy-link(line 141).Proposed cleanup
try { await navigator.clipboard.writeText(code); - const feedback = button.querySelector(".copy-feedback"); - if (feedback) { - feedback.textContent = "Copied!"; - } button.classList.add('copied'); setTimeout(() => { button.classList.remove('copied'); - }, 2000); + }, 1000); } catch (err) { console.error('Failed to copy:', err); }
🧹 Nitpick comments (2)
themes/esphome-theme/assets/js/main.js (1)
135-143: Clean implementation; consider adding error handling.The simplified approach using the
copiedclass is cleaner than animation-based feedback. The 1-second feedback duration is appropriate.Optional: Add error handling for clipboard operations
document.querySelectorAll('.copy-link').forEach(button => { button.addEventListener('click', async () => { const anchor = button.getAttribute('data-anchor'); const url = `${window.location.origin}${window.location.pathname}#${anchor}`; - await navigator.clipboard.writeText(url); - button.classList.add('copied'); - setTimeout(() => button.classList.remove('copied'), 1000); + try { + await navigator.clipboard.writeText(url); + button.classList.add('copied'); + setTimeout(() => button.classList.remove('copied'), 1000); + } catch (err) { + console.error('Failed to copy link:', err); + } }); });themes/esphome-theme/assets/css/main.css (1)
1776-1784: Consider refactoring to avoid !important declarations.The
!importantflags on lines 1777 and 1782 suggest specificity conflicts. While functional, this approach can make future maintenance harder.Optional: Investigate specificity and remove !important if possible
Consider increasing selector specificity instead:
.component-card a h2, .component-card a h3, .component-card a p, .component-card a { color: var(--component-card-text); text-decoration: none; } .component-card a:hover { color: var(--component-card-text); text-decoration: none; }Or restructure to avoid conflicts altogether by ensuring component-card styles have adequate specificity naturally.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
static/fonts/Figtree-Italic.woff2is excluded by!**/*.woff2static/fonts/Figtree.woff2is excluded by!**/*.woff2static/fonts/InstrumentSans-Italic.woff2is excluded by!**/*.woff2static/fonts/InstrumentSans.woff2is excluded by!**/*.woff2static/images/icons/content-copy.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
content/guides/getting_started_hassio.mdthemes/esphome-theme/assets/css/chroma.cssthemes/esphome-theme/assets/css/main.cssthemes/esphome-theme/assets/js/main.jsthemes/esphome-theme/layouts/_default/_markup/render-codeblock.htmlthemes/esphome-theme/layouts/_default/baseof.html
🚧 Files skipped from review as they are similar to previous changes (1)
- content/guides/getting_started_hassio.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
themes/esphome-theme/layouts/_default/_markup/render-codeblock.htmlthemes/esphome-theme/layouts/_default/baseof.htmlthemes/esphome-theme/assets/js/main.jsthemes/esphome-theme/assets/css/main.cssthemes/esphome-theme/assets/css/chroma.css
🧠 Learnings (1)
📚 Learning: 2025-05-12T00:02:50.869Z
Learnt from: jesserockz
Repo: esphome/esphome-docs PR: 4901
File: changelog/2025.4.0.rst:124-126
Timestamp: 2025-05-12T00:02:50.869Z
Learning: In the ESPHome changelog structure, the "All changes" section should have the `:open:` attribute to be expanded by default, but the "Dependency Changes" section should NOT have this attribute as it should remain collapsed by default.
Applied to files:
themes/esphome-theme/assets/js/main.js
🧬 Code graph analysis (1)
themes/esphome-theme/assets/js/main.js (1)
themes/esphome-theme/assets/js/search.js (2)
url(72-72)navContainer(189-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (20)
themes/esphome-theme/layouts/_default/_markup/render-codeblock.html (1)
2-9: LGTM! Clean icon toggle implementation.The dual-SVG approach with
fill="currentColor"and distinct classes enables smooth state transitions via CSS. Thearia-labelprovides proper accessibility.themes/esphome-theme/assets/css/chroma.css (2)
1-76: LGTM! Well-structured light theme syntax highlighting.The Hugo-generated styles use
--surface-colorfor theme-aware backgrounds and provide comprehensive token mappings. Line number styling withuser-select: noneprevents unwanted selection during copy operations.
77-154: LGTM! Comprehensive dark theme syntax highlighting.The monokai-based dark theme uses
--sidebar-backgroundfor the code block background and provides complete token coverage. The additional generic tokens (GenericDeleted, GenericInserted) enhance diff/patch rendering.themes/esphome-theme/assets/js/main.js (2)
116-121: LGTM! Defensive fix for empty summary elements.The implementation correctly identifies and fixes empty summary elements by setting a default text. The
trim()check properly handles whitespace-only content.
280-354: Excellent scroll-spy implementation with proper state management.The IntersectionObserver-based scroll-spy correctly identifies the topmost visible heading and updates the active TOC link. The pause/resume mechanism during programmatic scrolling prevents interference and race conditions.
The 500ms resume delay (line 349) appears reasonable for typical smooth-scroll durations, though it's somewhat arbitrary. Consider tuning if users experience TOC state flickering after navigation.
themes/esphome-theme/layouts/_default/baseof.html (7)
97-97: LGTM! Version badge enhances discoverability.The nav-version link provides quick access to the changelog and displays the current release version. The implementation correctly uses site data for the version text.
100-101: LGTM! Explicit icon markup improves clarity.The inline SVGs for menu/close icons are loaded via
os.ReadFile, eliminating additional HTTP requests. The CSS-based visibility toggle (controlled byaria-expanded) maintains accessibility.
104-113: LGTM! Appropriate theme controls for different contexts.The desktop toggle button and mobile picker provide suitable UX patterns for their respective contexts. The explicit theme-option buttons in the mobile picker improve discoverability on smaller screens.
130-132: LGTM! Icon-based search UI is more polished.The inline search icon provides a cleaner, more modern search interface. The container structure supports the pagefind integration and allows for proper icon/input alignment via CSS.
138-138: Overlay element added for mobile interactions.The overlay provides a backdrop for mobile menu/TOC interactions. Note that AI summaries indicate this element is removed in some other layout files, which may be intentional for different page types.
140-140: LGTM! Conditional homepage class enables targeted styling.The conditional class application allows for homepage-specific layout adjustments while maintaining a common base template structure.
151-155: LGTM! Theme-aware footer branding ensures optimal contrast.Providing separate light/dark logo variants ensures proper contrast and readability across both themes. The CSS-based visibility toggle (
.light-only/.dark-only) maintains a clean implementation.themes/esphome-theme/assets/css/main.css (8)
1-32: LGTM! Well-configured self-hosted fonts.The self-hosted WOFF2 fonts with variable weight ranges and
font-display: swapprovide optimal performance and eliminate external dependencies. The absolute paths ensure correct loading from any page depth.
35-38: LGTM! Prevents animation jank during theme switches.The
.theme-transition-disabledclass with!importantensures instant color changes during theme switching, preventing distracting transition animations.
40-121: LGTM! Comprehensive design token system for light theme.The well-organized CSS variables provide semantic naming, clear hierarchy, and backward compatibility aliases. The design tokens cover all major UI elements and will ease future theme customization.
123-169: LGTM! Dark theme variables provide appropriate contrast.The dark theme overrides maintain proper contrast ratios with lighter text on dark backgrounds. The increased shadow opacity and adjusted hover states are appropriate for the dark aesthetic.
639-657: LGTM! Version badge styling is clean and unobtrusive.The badge styling uses appropriate design tokens and provides subtle hover feedback. The small font size and badge appearance keep it from dominating the navigation.
2163-2240: Modern CSS features with appropriate fallbacks.The progressive enhancement approach using
@supportsensures graceful degradation for browsers withoutinterpolate-sizeor::details-content. The custom chevron indicator and smooth transitions enhance UX without breaking compatibility.
2242-2289: LGTM! Copy button styling aligns with updated markup.The CSS correctly toggles between copy and check icons using the
.copiedclass, matching the dual-SVG implementation inrender-codeblock.html. Hover and active states provide clear visual feedback.
1866-2015: LGTM! Mobile theme picker provides clear visual feedback.The mobile-specific theme picker with explicit Light/Dark options is well-styled with clear active states. The separation between desktop toggle and mobile picker ensures appropriate UX for each context.
- Remove dead code for .copy-feedback element (no longer in markup) - Normalize timeout to 1000ms for both copy handlers - Add try/catch error handling to copy-link handler
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
themes/esphome-theme/assets/js/main.js
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
themes/esphome-theme/assets/js/main.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: esphome/esphome-docs PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T00:13:13.459Z
Learning: Open pull requests against the correct base branch: use `current` for documentation-only revisions and `next` for changes tied to new features in esphome/esphome
📚 Learning: 2025-05-12T00:02:50.869Z
Learnt from: jesserockz
Repo: esphome/esphome-docs PR: 4901
File: changelog/2025.4.0.rst:124-126
Timestamp: 2025-05-12T00:02:50.869Z
Learning: In the ESPHome changelog structure, the "All changes" section should have the `:open:` attribute to be expanded by default, but the "Dependency Changes" section should NOT have this attribute as it should remain collapsed by default.
Applied to files:
themes/esphome-theme/assets/js/main.js
🧬 Code graph analysis (1)
themes/esphome-theme/assets/js/main.js (1)
themes/esphome-theme/assets/js/search.js (2)
url(72-72)navContainer(189-189)
🔇 Additional comments (5)
themes/esphome-theme/assets/js/main.js (5)
116-121: LGTM!Good defensive fix for empty
<summary>elements, ensuring accessibility and usability when summaries lack text content.
139-145: LGTM!Good addition of error handling for the clipboard API. The try/catch properly guards against permission denials or other clipboard failures.
151-169: LGTM!Copy button functionality is correctly implemented with consistent 1000ms feedback timing across both copy handlers.
271-278: LGTM!The expanded selector correctly includes both
.toc-entryand.toc-secondaryclasses, consistent with the scroll-spy query on line 299.
280-316: LGTM!The IntersectionObserver-based scroll-spy is well-implemented:
- Proper guard against empty arrays before
reduce- Efficient single
currentActivetracking avoids DOM thrashingrootMargincorrectly accounts for the fixed header offset
rjt-rockx
changed the title
Looked into this a bit and I'm not sure it'd be feasible to enforce that as a lint rule since we're factoring in names and grammar and such. It's more a stylistic decision to prefer sentence case over title case really, and while it might make sense to document that in a style guide or design system, I feel we'd just end up dealing with a ton of false positives and such @stuartparmenter |
- Add spacing tokens (--space-xs through --space-xl) - Add icon size tokens (--icon-sm, --icon-md) - Add border radius tokens (--radius-sm, --radius-md, --radius-lg) - Convert layout values from px to rem (nav-height, max-page-width, sidebar) - Replace hardcoded border-radius with token variables - Update component cards to horizontal layout with line-clamped text
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
themes/esphome-theme/assets/css/main.css (1)
198-218: Consider removing duplicateoverscroll-behaviordeclaration.
overscroll-behavior: noneis declared on bothhtml(line 201) andbody(line 215). Setting it onhtmlalone is typically sufficient. Consider removing the duplicate for cleaner code.🔎 Suggested change
body { min-height: 100vh; margin: 0; font-family: var(--font-body); line-height: 1.6; color: var(--text-color); background-color: var(--background-color); - overscroll-behavior: none; display: flex; flex-direction: column; }themes/esphome-theme/assets/js/main.js (1)
319-355: Scroll spy implementation is sound; consider usinggetBoundingClientRect()for more robust positioning.The
closeTOC()function is defined inmenu.js(line 41) and is globally accessible, so the call at line 323 works correctly.The offset calculation at line 340 using
targetElement.offsetTopfunctions properly but could be more robust. Consider usinggetBoundingClientRect()instead, which accounts for layout changes more reliably:top: targetElement.getBoundingClientRect().top + window.scrollY - HEADER_HEIGHTThis approach is less susceptible to issues if the layout ever changes such that
offsetParentis not the document body.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
themes/esphome-theme/assets/css/main.cssthemes/esphome-theme/assets/js/main.js
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
themes/esphome-theme/assets/js/main.jsthemes/esphome-theme/assets/css/main.css
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: esphome/esphome-docs PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T00:13:13.459Z
Learning: Open pull requests against the correct base branch: use `current` for documentation-only revisions and `next` for changes tied to new features in esphome/esphome
📚 Learning: 2025-05-12T00:02:50.869Z
Learnt from: jesserockz
Repo: esphome/esphome-docs PR: 4901
File: changelog/2025.4.0.rst:124-126
Timestamp: 2025-05-12T00:02:50.869Z
Learning: In the ESPHome changelog structure, the "All changes" section should have the `:open:` attribute to be expanded by default, but the "Dependency Changes" section should NOT have this attribute as it should remain collapsed by default.
Applied to files:
themes/esphome-theme/assets/js/main.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (15)
themes/esphome-theme/assets/js/main.js (4)
116-121: LGTM!Good accessibility fix to ensure empty
<summary>elements have meaningful text for screen readers and users.
139-145: LGTM!Good error handling for the clipboard API with proper visual feedback timing.
162-164: LGTM!Consistent 1-second feedback timing across copy interactions.
272-273: LGTM!Good extraction of
HEADER_HEIGHTconstant for maintainability, and expanded TOC selector to support the new TOC structure.themes/esphome-theme/assets/css/main.css (11)
1-33: LGTM!Well-structured font-face declarations with
font-display: swapfor good loading performance and variable font weights to reduce HTTP requests.
35-38: LGTM!The
!importantdeclaration is justified here to reliably disable transitions during theme switches, preventing visual artifacts.
40-137: LGTM!Excellent design token system with clear categorization, consistent naming, and backward-compatibility aliases. Using
remunits for spacing improves accessibility.
139-189: LGTM!Dark theme variables are well-structured with appropriate contrast adjustments and increased shadow opacity for visibility on dark backgrounds.
525-594: LGTM!Clean TOC styling with smooth transitions and clear visual hierarchy. The border-left indicator for active state provides good visual feedback.
863-867: Modern:has()selector usage looks good.The
:has()selector for focus expansion has broad browser support (Chrome 105+, Safari 15.4+, Firefox 121+). If support for older browsers is needed, the search box remains functional without the expansion effect, providing graceful degradation.
1771-1796: LGTM!Correct implementation of multi-line text truncation with
-webkit-line-clamp. The required-webkit-boxdisplay and-webkit-box-orient: verticalproperties are properly paired.
2190-2267: Good progressive enhancement for<details>animations.The
@supportsblocks forinterpolate-sizeand::details-contentprovide graceful degradation — unsupported browsers get functional disclosure widgets without animations. Theprefers-reduced-motionquery inside the::details-contentblock is a nice accessibility touch.
1334-1383: LGTM!Well-structured button system with clear primary/secondary variants and proper handling of both
<button>and<a>elements.
1942-1965: LGTM!The
clip-pathanimation for the mobile menu is a performant choice that enables smooth reveal animations. Thepointer-eventstoggle properly prevents interaction with hidden menu items.
2269-2315: LGTM!Clean copy button styling with proper icon swap on the
copiedstate, matching the JavaScript implementation.
2 participants
Description
Redesign of the documentation site styling, focusing on improved readability, performance, and maintainability.
What changed:
@supportswrappers forinterpolate-size,::details-content)Related issue (if applicable): N/A
Pull request in esphome with YAML changes (if applicable): N/A
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.mdwhen creating new documents for new components or cookbook.