chore: fix navigation to home#798
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
it's intentional, to reduce clicks when searching. what we could do is add an additional search button in the header, that expands into the search bar when clicked. it could be located next to the mobile menu button on the right |
7735fea to
ea88a00
Compare
|
@danielroe , I think I've done it now.
|
0ba62bb to
34ed378
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe AppHeader.vue component has been refactored to improve mobile navigation handling. The mobile logo element was converted from a non-functional button to a NuxtLink for home navigation with updated accessibility labels. The inline search icon was removed from the logo area. A new mobile search trigger button (ButtonBase) was introduced, appearing conditionally when not on the home page and when search is not expanded. The desktop layout structure remains unchanged, though the logo section now consistently uses NuxtLink for navigation. Minor CSS class adjustments were made to the nav container. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/AppHeader.vue (1)
276-284: Consider placingv-ifbefore other attributes for consistency.Vue style guides typically recommend placing directives like
v-ifnear the top of the attribute list for better readability. This is a minor style suggestion.♻️ Suggested reordering
<ButtonBase + v-if="!isSearchExpanded && !isOnHomePage" type="button" class="sm:hidden ms-auto" :aria-label="$t('nav.tap_to_search')" :aria-expanded="isSearchExpanded" `@click`="expandMobileSearch" - v-if="!isSearchExpanded && !isOnHomePage" classicon="i-carbon:search" />
| <!-- Mobile: Search button (expands search) --> | ||
| <ButtonBase | ||
| type="button" | ||
| class="sm:hidden ms-auto" | ||
| :aria-label="$t('nav.tap_to_search')" | ||
| :aria-expanded="showMobileMenu" | ||
| @click="expandMobileSearch" | ||
| v-if="!isSearchExpanded && !isOnHomePage" | ||
| classicon="i-carbon:search" | ||
| /> |
There was a problem hiding this comment.
Incorrect aria-expanded binding on mobile search button.
The aria-expanded attribute is bound to showMobileMenu, but this button controls search expansion, not the mobile menu. Screen readers will announce the wrong state.
🐛 Proposed fix
<ButtonBase
type="button"
class="sm:hidden ms-auto"
:aria-label="$t('nav.tap_to_search')"
- :aria-expanded="showMobileMenu"
+ :aria-expanded="isSearchExpanded"
`@click`="expandMobileSearch"
v-if="!isSearchExpanded && !isOnHomePage"
classicon="i-carbon:search"
/>📝 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.
| <!-- Mobile: Search button (expands search) --> | |
| <ButtonBase | |
| type="button" | |
| class="sm:hidden ms-auto" | |
| :aria-label="$t('nav.tap_to_search')" | |
| :aria-expanded="showMobileMenu" | |
| @click="expandMobileSearch" | |
| v-if="!isSearchExpanded && !isOnHomePage" | |
| classicon="i-carbon:search" | |
| /> | |
| <!-- Mobile: Search button (expands search) --> | |
| <ButtonBase | |
| type="button" | |
| class="sm:hidden ms-auto" | |
| :aria-label="$t('nav.tap_to_search')" | |
| :aria-expanded="isSearchExpanded" | |
| `@click`="expandMobileSearch" | |
| v-if="!isSearchExpanded && !isOnHomePage" | |
| classicon="i-carbon:search" | |
| /> |
|
Thanks @danielroe , @ghostdevv . |

This is for the mobile version.
In the Navbar we get logo + search icon in any page like "/about", "/compare" etc. When we click on the search icon it expands a searchbar. Currently when we click on the logo, it also expands a searchbar. I mean I think when we click on logo we should navigate to "/" page. In the desktop version, when we click on the logo, it navigates to "/" page.