Conversation
|
The argument could be made that it's easy to close the navbar to uncover the content, and shifting all the content down suddenly is too jarring. |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the navbar and header styling and behavior to improve code maintainability and fix mobile layout issues. The changes introduce CSS variables for consistent sizing, reorganize CSS rules into mobile/desktop specific sections, and simplify JavaScript navigation toggle logic.
Changes:
- Introduced CSS variables (
--header-height,--navbar-width) for better maintainability - Removed fixed positioning from navbar in mobile view to prevent content overlay
- Simplified hamburger menu toggle logic from fadeIn/fadeOut to toggle()
- Removed unused
.vertical-alignCSS class and click-outside-to-close functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| webroot/js/global-late.js | Simplified navbar toggle logic and removed click-outside-to-close behavior |
| webroot/css/navbar.css | Added CSS variables, reorganized mobile/desktop styles, adjusted positioning |
| webroot/css/global.css | Removed unused .vertical-align CSS class |
| resources/templates/header.php | Removed .vertical-align class from hamburger button, reformatted HTML |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| width: var(--navbar-width); | ||
| padding-top: var(--header-height); | ||
| height: 100%; |
There was a problem hiding this comment.
The fixed positioned nav.mainNav is missing left: 0 and top: 0 properties that were removed from the original code. Without left: 0, the navbar may not be anchored to the left edge of the viewport. Additionally, using height: 100% with padding-top: var(--header-height) will cause the total element height to exceed the viewport (100% + 50px), creating unwanted overflow. Consider restoring left: 0 and using either top: 0; bottom: 0; (removing the height property) or height: calc(100% - var(--header-height)) instead.
| width: var(--navbar-width); | |
| padding-top: var(--header-height); | |
| height: 100%; | |
| top: 0; | |
| left: 0; | |
| bottom: 0; | |
| width: var(--navbar-width); | |
| padding-top: var(--header-height); |
--header-height--navbar-widthfor readabilityposition: fixedon the navbar in mobile view so it doesn't cover up any contentposition: absolutefrom the hamburger button, usedfloat: rightinsteadfadeOut/fadeInto usetoggleinsteadbefore:
Screen.Recording.2026-01-12.at.11.56.37.AM.mov
after: