-
Notifications
You must be signed in to change notification settings - Fork 13k
refactor: replace legacy Menu with GenericMenu
#38287
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: develop
Are you sure you want to change the base?
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughReplaces legacy Menu/Option usages with GenericMenu across audit, marketplace, and omnichannel UIs; converts keyed preset/option objects into flat item arrays with id/icon/content/onClick handlers; adjusts some layouts and filter grouping; adds a translation key and updates e2e test/menu page-object helpers to target new menu structure. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
46726fc to
5aea96e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38287 +/- ##
===========================================
+ Coverage 70.74% 70.75% +0.01%
===========================================
Files 3159 3159
Lines 109384 109355 -29
Branches 19676 19687 +11
===========================================
- Hits 77383 77376 -7
+ Misses 29963 29942 -21
+ Partials 2038 2037 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d5549c6 to
f0c0f1e
Compare
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.
No issues found across 11 files
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/AppLogsFilter.tsx`:
- Around line 94-97: The Severity label isn't associated with its input, so add
an accessible link: set the Label's htmlFor to 'severityFilter' (Label
htmlFor='severityFilter') and ensure the SeverityFilterSelect component accepts
and forwards the id and ref it receives from the Controller (it already receives
id='severityFilter' via {...field} — verify SeverityFilterSelect forwards that
id to the underlying HTML select/input and uses React.forwardRef to forward the
ref so screen readers and form libraries can associate the label and control).
🧹 Nitpick comments (1)
apps/meteor/tests/e2e/page-objects/omnichannel-departments.ts (1)
92-106: Consider extracting the repeated menu locator for reuse.The
getByRole('menu', { name: 'Options' })locator is repeated across all four menu option getters. Per coding guidelines, commonly used locators should be stored in variables/constants for reuse.♻️ Suggested refactor
+ private get optionsMenu() { + return this.page.getByRole('menu', { name: 'Options' }); + } + get menuEditOption() { - return this.page.getByRole('menu', { name: 'Options' }).getByRole('menuitem', { name: 'Edit' }); + return this.optionsMenu.getByRole('menuitem', { name: 'Edit' }); } get menuDeleteOption() { - return this.page.getByRole('menu', { name: 'Options' }).getByRole('menuitem', { name: 'Delete' }); + return this.optionsMenu.getByRole('menuitem', { name: 'Delete' }); } get menuArchiveOption() { - return this.page.getByRole('menu', { name: 'Options' }).getByRole('menuitem', { name: 'Archive' }); + return this.optionsMenu.getByRole('menuitem', { name: 'Archive' }); } get menuUnarchiveOption() { - return this.page.getByRole('menu', { name: 'Options' }).getByRole('menuitem', { name: 'Unarchive' }); + return this.optionsMenu.getByRole('menuitem', { name: 'Unarchive' }); }Based on coding guidelines, commonly used locators should be stored in variables/constants for reuse.
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/AppLogsFilter.tsx
Show resolved
Hide resolved
GenericMenuGenericMenu
GenericMenuMenu with GenericMenu
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppLogs/Filters/AppLogsFilter.tsx
Show resolved
Hide resolved
dougfabris
left a comment
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.
LGTM!
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
2b02641 to
996bda3
Compare
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/tests/e2e/omnichannel/omnichannel-departaments.spec.ts (1)
243-260: Wait for menu dismissal afterEscapeto prevent flaky overlaysLine 247 and Line 259 close the menu via
Escape, but the test proceeds immediately. If the menu animates or overlays the page, subsequent steps can intermittently fail. Add an explicit expectation that the menu item is hidden before moving on.🔧 Suggested fix
- await page.keyboard.press('Escape'); + await page.keyboard.press('Escape'); + await expect(poOmnichannelDepartments.menuDeleteOption).not.toBeVisible();- await page.keyboard.press('Escape'); + await page.keyboard.press('Escape'); + await expect(poOmnichannelDepartments.menuDeleteOption).not.toBeVisible();
🧹 Nitpick comments (2)
apps/meteor/client/views/audit/components/forms/DateRangePicker.tsx (1)
186-194: Consider centralizingDateRangePickercomponents.There appears to be a similar
DateRangePickerin the omnichannel module with identical preset logic. This could be consolidated into a shared component to reduce duplication.apps/meteor/client/views/omnichannel/departments/DepartmentsTable/DepartmentItemMenu.tsx (1)
22-23: Remove outdated TODO comment.The TODO referencing "MenuV2" is now stale since this PR migrates to
GenericMenu. Per coding guidelines, code comments should be avoided in the implementation.Proposed fix
-// TODO: Use MenuV2 instead of Menu const DepartmentItemMenu = ({ department, archived }: DepartmentItemMenuProps): ReactElement => {
CORE-1749
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
New Features
Tests
Localization
✏️ Tip: You can customize this high-level summary in your review settings.