fix scroll sub menu when current page is not in the current sub-menu … - #2706
fix scroll sub menu when current page is not in the current sub-menu …#2706benjgil wants to merge 3 commits into
Conversation
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 0195139 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe menu category enables its dropdown before presenting it. A regression test verifies scrolling and last-item visibility for a dropdown with 19 inactive items. A patch changeset documents the fix. ChangesMenu category dropdown
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/menu-category/test/menu-category.ct.ts`:
- Around line 587-595: Update the scrollability test around menuCategory to
exercise the interaction path: scroll the final non-parent menu item into the
dropdown’s visible area, click it, and assert that the dropdown closes. Retain
the overflow assertion, but replace the visibility-only check with assertions
that validate the user-facing selection behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02b26243-59e8-4ba9-beb3-a83b677fb8a0
📒 Files selected for processing (2)
packages/core/src/components/menu-category/menu-category.tsxpackages/core/src/components/menu-category/test/menu-category.ct.ts
| const isScrollable = await dropdown.evaluate((el) => { | ||
| return el.scrollHeight > el.clientHeight; | ||
| }); | ||
| expect(isScrollable).toBe(true); | ||
|
|
||
| const lastItem = menuCategory | ||
| .locator('ix-menu-item:not(.category-parent)') | ||
| .last(); | ||
| await expect(lastItem).not.toBeHidden(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Exercise the scroll and click path.
scrollHeight > clientHeight only proves that overflow exists. not.toBeHidden() also passes when the final item is outside the visible scrollport. Scroll the final item into view, click it, and assert the dropdown closes. This validates the user interaction fixed by this PR.
Proposed test update
expect(isScrollable).toBe(true);
const lastItem = menuCategory
.locator('ix-menu-item:not(.category-parent)')
.last();
- await expect(lastItem).not.toBeHidden();
+ await lastItem.scrollIntoViewIfNeeded();
+ await expect(lastItem).toBeInViewport();
+ await lastItem.click();
+ await expect(dropdown).not.toBeVisible();As per coding guidelines, update tests for user-facing behavior changes and use core component tests for interaction behavior.
📝 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.
| const isScrollable = await dropdown.evaluate((el) => { | |
| return el.scrollHeight > el.clientHeight; | |
| }); | |
| expect(isScrollable).toBe(true); | |
| const lastItem = menuCategory | |
| .locator('ix-menu-item:not(.category-parent)') | |
| .last(); | |
| await expect(lastItem).not.toBeHidden(); | |
| const isScrollable = await dropdown.evaluate((el) => { | |
| return el.scrollHeight > el.clientHeight; | |
| }); | |
| expect(isScrollable).toBe(true); | |
| const lastItem = menuCategory | |
| .locator('ix-menu-item:not(.category-parent)') | |
| .last(); | |
| await lastItem.scrollIntoViewIfNeeded(); | |
| await expect(lastItem).toBeInViewport(); | |
| await lastItem.click(); | |
| await expect(dropdown).not.toBeVisible(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/components/menu-category/test/menu-category.ct.ts` around
lines 587 - 595, Update the scrollability test around menuCategory to exercise
the interaction path: scroll the final non-parent menu item into the dropdown’s
visible area, click it, and assert that the dropdown closes. Retain the overflow
assertion, but replace the visibility-only check with assertions that validate
the user-facing selection behavior.
Source: Coding guidelines
|
1 participant
💡 What is the current behavior?
If an application sub-menu contains enough items to create a scroll, the scroll is only visible when the user is on a page that is in that sub-menu. If the user is on a page that is NOT inside the long sub-menu, and then hovers on the main nav menu item that contains the sub-menu, then the sub-menu scroll is not visible and items in the sub-menu are not clickable
GitHub Issue Number: #2664
🆕 What is the new behavior?
Fix the bug by setting showDropdown to true before the dropdown children and scroll are calculated
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
Bug Fixes
Tests