fix: fix search layout stability and mobile close on outside click#2204
fix: fix search layout stability and mobile close on outside click#2204thegreatalxx wants to merge 3 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes modify the AppHeader component to address layout stability and user interaction issues. The component now includes a click-outside handler to collapse the mobile search when users click outside the search area on non-search pages. The visibility handling for navigation items has been adjusted from Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (2)
test/nuxt/components/AppHeader.spec.ts (2)
11-15: Strengthen the search input assertion.The assertion
expect(html).toContain('search')is fragile as it matches any occurrence of the string 'search' (class names, attributes, text). Consider querying for the actual search input element.♻️ Suggested improvement
- const html = wrapper.html() - // The header renders a search form/input - expect(html).toContain('search') + // The search container with ref="searchContainerRef" should be rendered + // and contain an input element for the search functionality + const searchInput = wrapper.find('input[type="search"], input[placeholder*="search" i]') + expect(searchInput.exists()).toBe(true)
18-31: Test does not verify the behaviour described in its name.The test is named "nav links use invisible instead of hidden to prevent layout shift" but the assertion only checks that a nav list element exists. It doesn't verify:
- That
invisible pointer-events-noneclasses are applied whenshowFullSearchis true- That
hiddenis not used for this visibility togglingTo properly test this, you would need to mock
useConnectorto return a connected user (since the ul requiresisConnected && npmUser) and trigger a search focus to setshowFullSearch = true.♻️ Suggested improvement (simplified approach)
If full integration testing is not feasible, consider either:
- Renaming the test to reflect what it actually checks (e.g., "renders nav list element")
- Or adding a comment explaining the limitation
- it('nav links use invisible instead of hidden to prevent layout shift', async () => { + it('renders nav list structure for connected users', async () => { const wrapper = await mountSuspended(AppHeader, { route: '/', }) const html = wrapper.html() - // The nav list should use 'invisible pointer-events-none' (not 'hidden') - // when the search is expanded, to prevent layout shifts - // Verify the nav list element exists at all + // Verify the nav structure is present + // Note: Full visibility toggling behaviour requires mocking useConnector const navList = wrapper.find('nav ul, ul[class*="list-none"]') - // When not on a search page and search is not expanded, - // the nav list should be visible (not invisible) expect(navList.exists() || html.includes('list-none')).toBe(true) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ebf90a5-dfb6-49f9-a03f-a0e9a1843908
📒 Files selected for processing (2)
app/components/AppHeader.vuetest/nuxt/components/AppHeader.spec.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Hello. I will be closing this because there are already open or merged PRs to resolve the linked issues (check for linked PRs in those issues for details). We like to honour existing work when we can. Please check before making new PRs. Also, make sure to check if bugs exist on the preview URL: https://main.npmx.dev |
MatteoGabriele
added
the
007
3 participants
🔗 Linked issue
resolves #2191
resolves #2112
resolves #1881
🧭 Context
The AppHeader had multiple related layout issues: (1) the search container lacked a template ref so clicks outside it on mobile couldn't close the expanded search, (2) hiding the nav list with
hiddencaused a layout shift when the search expanded, and (3) the header row had an unnecessaryz-1class causing z-index conflicts with dropdowns.📚 Description
ref="searchContainerRef"to the search container and usedonClickOutsideto collapse the mobile search when clicking outside it.hiddentoinvisible pointer-events-noneso it still occupies space and prevents layout shifts when the search expands.z-1from the header container div to avoid stacking context conflicts with dropdown overlays.A regression test (
test/nuxt/components/AppHeader.spec.ts) mounts the header and verifies the search area renders and the header element is present.