-
Notifications
You must be signed in to change notification settings - Fork 40
feat: enhance homepage header with search functionality/visibility #234
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the homepage header by adding a search component and making both the search bar and the "Aspire" title appear in the header when scrolling past the hero section. The implementation removes a previously hidden datetime display and adds scroll-based visibility toggling.
- Adds Search component import and integration into the homepage header
- Implements scroll-based visibility logic to show/hide search and title based on hero section position
- Removes the hidden datetime component that was previously in the title wrapper
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const searchWrapper = document.querySelector('[data-homepage-search]'); | ||
| const titleWrapper = document.querySelector('[data-homepage-title]'); | ||
| const siteTitleText = titleWrapper?.querySelector('.site-title .sr-only'); | ||
| if (!searchWrapper && !siteTitleText) return; |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The early return condition uses AND (&&) but should use OR (||). If either searchWrapper or siteTitleText is null, the function should return early since the scroll logic depends on both elements existing. With the current logic, the function continues even when one of the elements is missing, leading to potential issues when trying to add/remove classes from null elements.
| if (!searchWrapper && !siteTitleText) return; | |
| if (!searchWrapper || !siteTitleText) return; |
| checkScroll(); | ||
|
|
||
| // Check on scroll | ||
| window.addEventListener('scroll', checkScroll, { passive: true }); |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each time initHomepageHeader is called, a new scroll event listener is added without removing the previous one. This creates a memory leak and causes checkScroll to be called multiple times per scroll event after page transitions. Consider storing the listener reference and removing it before adding a new one, or use an AbortController to manage the listener lifecycle.
| } | ||
|
|
||
| function checkScroll() { | ||
| const heroRect = hero!.getBoundingClientRect(); |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using TypeScript non-null assertion operator (!) in a plain JavaScript context. Since this is not a TypeScript file, the exclamation mark should be removed. The null check has already been performed at line 45, so accessing hero is safe within this function scope without the assertion operator.
| const heroRect = hero!.getBoundingClientRect(); | |
| const heroRect = hero.getBoundingClientRect(); |
| // Check on initial load | ||
| checkScroll(); | ||
|
|
||
| // Check on scroll | ||
| window.addEventListener('scroll', checkScroll, { passive: true }); |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkScroll function is called on every scroll event without any throttling or debouncing. This can cause performance issues with frequent DOM operations (classList manipulations) during rapid scrolling. Consider implementing throttling or using requestAnimationFrame to optimize scroll performance.
| // Check on initial load | |
| checkScroll(); | |
| // Check on scroll | |
| window.addEventListener('scroll', checkScroll, { passive: true }); | |
| let ticking = false; | |
| function onScroll() { | |
| if (!ticking) { | |
| ticking = true; | |
| window.requestAnimationFrame(() => { | |
| checkScroll(); | |
| ticking = false; | |
| }); | |
| } | |
| } | |
| // Check on initial load | |
| checkScroll(); | |
| // Check on scroll (throttled with requestAnimationFrame) | |
| window.addEventListener('scroll', onScroll, { passive: true }); |
Related to #179