Fix menu behaviour on mobile devices. #5439
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes menu behavior on mobile devices by preventing sub-menus from being clipped on narrow screens. The changes modify both JavaScript and CSS to make sub-menus open inline instead of to the left of the top-level menu on mobile devices.
Key changes:
- JavaScript refactoring to make mobile detection reusable and fix selector issues
- CSS media query adjustments to change dropdown positioning behavior on narrow screens
- Improved mobile menu scrolling and overflow handling
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| themes/esphome-theme/assets/js/menu.js | Converts mobile detection to reusable function and fixes dropdown selector |
| themes/esphome-theme/assets/css/main.css | Reorganizes media queries and changes dropdown positioning for mobile devices |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCSS updates adjust dropdown/menu behavior: restore desktop hover display and add mobile-specific positioning, full-width responsiveness, overflow, and scrollbar handling. JS introduces isMobileScreen() and replaces prior isMobile checks in TOC and dropdown logic, updates dropdown button selector, and conditions behavior based on screen width. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser/UI
participant MenuJS as menu.js
participant DOM as DOM/CSS
rect rgba(232, 244, 253, 0.6)
note over User,DOM: Dropdown interaction
User->>Browser: Click dropdown button
Browser->>MenuJS: handleDropdownClick
MenuJS->>MenuJS: isMobileScreen()
alt Mobile screen
MenuJS->>DOM: Toggle dropdown open state (JS)
else Desktop screen
note over DOM: Display via CSS :hover
Browser-->>DOM: Apply hover styles
end
end
rect rgba(232, 244, 253, 0.6)
note over User,DOM: Table of Contents (TOC)
User->>Browser: Open/Close TOC
Browser->>MenuJS: openTOC / closeTOC
MenuJS->>MenuJS: isMobileScreen()
alt Mobile screen
MenuJS->>DOM: Apply mobile-specific open/close behavior
else Desktop screen
MenuJS->>DOM: Apply desktop-specific behavior
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
✅ Deploy Preview for esphome ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for esphome ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 (2)
themes/esphome-theme/assets/js/menu.js (2)
140-160: Inconsistent mobile detection—preferisMobileScreen().The
toggleDropdownandcloseAllDropdownsfunctions use directwindow.innerWidth > mobileWidthStopchecks, while other parts of the code use the newisMobileScreen()helper. This inconsistency creates maintenance risk.Apply this diff to use the helper consistently:
function toggleDropdown(button) { - if (window.innerWidth > mobileWidthStop) return; + if (!isMobileScreen()) return; const isExpanded = button.getAttribute('aria-expanded') === 'true'; closeAllDropdowns(); if (!isExpanded) { button.setAttribute('aria-expanded', 'true'); const dropdownContent = button.nextElementSibling; dropdownContent.style.display = 'block'; } } // Close all dropdowns function closeAllDropdowns() { - if (window.innerWidth > mobileWidthStop) return; + if (!isMobileScreen()) return; dropdownButtons.forEach(btn => { btn.setAttribute('aria-expanded', 'false'); const dropdownContent = btn.nextElementSibling; if (dropdownContent) dropdownContent.style.display = 'none'; }); }
177-188: Inconsistent mobile detection in event listeners.The outside-click handler (line 178) and resize handler (line 185) still use direct
window.innerWidth > mobileWidthStopchecks instead ofisMobileScreen().Apply this diff for consistency:
// Close menu on outside click (mobile only) document.addEventListener('click', function(e) { - if (window.innerWidth > mobileWidthStop) return; + if (!isMobileScreen()) return; if (!e.target.closest('.hamburger-button') && !e.target.closest('.nav-links')) { closeMenu(); } }); // Close menu on resize to desktop window.addEventListener('resize', function() { - if (window.innerWidth > mobileWidthStop) { + if (!isMobileScreen()) { closeMenu(); } });
🧹 Nitpick comments (3)
themes/esphome-theme/assets/css/main.css (3)
1317-1318: Consider accessibility implications of hiding scrollbars.Hiding scrollbars (
scrollbar-width: nonefor Firefox,-ms-overflow-style: nonefor IE) reduces visual clutter but may prevent users from realizing that the navigation menu is scrollable, especially on mobile devices with limited viewport height.Consider scoping the scrollbar hiding specifically to
.nav-links.activeto match the overflow behavior:-.nav-links { +.nav-links.active { scrollbar-width: none; /* Firefox */ -ms-overflow-style: none; /* Internet Explorer 10+ */ }Alternatively, provide a visual cue (fade effect, scroll indicator) to signal scrollable content.
1344-1352: Magic number200pxlacks explanation.Line 1347 positions the dropdown content with
right: 200pxon mobile screens (between 700px and 1000px wide). This specific offset is not explained and resembles the previous concern about magic numbers.Consider:
- Adding a CSS comment to explain why 200px is needed
- Defining it as a CSS custom property (e.g.,
--dropdown-offset-mobile) for better maintainability- Verifying whether this offset is optimal across different mobile device widths
Example:
.dropdown-content { position: absolute; top: 0; right: 200px; /* Offset to prevent clipping on medium mobile screens */ box-shadow: none; margin-top: 0.5em; padding-left: 1em; padding-right: 1em; }
1254-1268: Review the 700px narrow-screen breakpoint
This custommax-width: 700pxtier sits between typical mobile (~600px) and tablet (~768px) thresholds. Ensure your nav truly needs inline dropdowns at this exact point—either justify it with content-driven wrapping tests or align with a more common breakpoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
themes/esphome-theme/assets/css/main.css(2 hunks)themes/esphome-theme/assets/js/menu.js(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
- Do not generate or add any sequence diagrams
Files:
themes/esphome-theme/assets/js/menu.jsthemes/esphome-theme/assets/css/main.css
🔇 Additional comments (3)
themes/esphome-theme/assets/js/menu.js (2)
4-6: LGTM! Clean mobile detection abstraction.The
isMobileScreen()function properly encapsulates the mobile width check by reading the CSS custom property--mobile-width-stop(1000px) and comparing it with the current window width.
90-90: Selector change is correct..dropdown buttonmatches the existing<button class="dropbtn">elements and the click handler’s.dropbtncheck remains valid—no further updates needed.themes/esphome-theme/assets/css/main.css (1)
1249-1251: LGTM! Hover behavior restored for desktop.This correctly restores the hover-based dropdown display for wide screens (min-width: 1001px), ensuring desktop users can interact with dropdowns via mouse hover while mobile users rely on click handlers from the JS file.
| scrollbar-width: none; /* Firefox */ | ||
| -ms-overflow-style: none; /* Internet Explorer 10+ */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add WebKit scrollbar hiding for Chrome/Safari.
The code hides scrollbars for Firefox (scrollbar-width: none) and IE (-ms-overflow-style: none) but omits WebKit-based browsers (Chrome, Safari, Edge). This creates an inconsistent experience.
Add the WebKit scrollbar rule:
.nav-links {
scrollbar-width: none; /* Firefox */
-ms-overflow-style: none; /* Internet Explorer 10+ */
}
+.nav-links::-webkit-scrollbar {
+ display: none; /* Chrome, Safari, Edge */
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scrollbar-width: none; /* Firefox */ | |
| -ms-overflow-style: none; /* Internet Explorer 10+ */ | |
| .nav-links { | |
| scrollbar-width: none; /* Firefox */ | |
| -ms-overflow-style: none; /* Internet Explorer 10+ */ | |
| } | |
| .nav-links::-webkit-scrollbar { | |
| display: none; /* Chrome, Safari, Edge */ | |
| } |
🤖 Prompt for AI Agents
themes/esphome-theme/assets/css/main.css around lines 1317-1318: the CSS hides
scrollbars for Firefox and IE but not for WebKit browsers; add a rule targeting
WebKit scrollbars (the ::-webkit-scrollbar pseudo-element) immediately after the
existing rules to hide the scrollbar for Chrome/Safari/Edge (e.g., set the
scrollbar pseudo-element to not display or zero width/height), keeping it scoped
consistently with the surrounding selector.
✅ Deploy Preview for esphome ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Description:
On a mobile device with a narrow screen the sub-menus get clipped. Revise the css so that on such devices the submenu opens in-line instead of to the left of the top-level menu.
Related issue (if applicable): fixes
Pull request in esphome with YAML changes (if applicable):
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.New Component Images
If you are adding a new component to ESPHome, you can automatically generate a standardized black and white component name image for the documentation.
To generate a component image:
Comment on this pull request with the following command, replacing
COMPONENT_NAMEwith your component name in UPPER_CASE format with underscores (e.g.,BME280,SHT3X,DALLAS_TEMP):The ESPHome bot will respond with a downloadable ZIP file containing the SVG image.
Extract the SVG file and place it in the
images/folder of this repository.Use the image in your component's index table entry in
/components/index.rst.Example: For a component called "DHT22 Temperature Sensor", use: