component(ContextMenuViewport): move to ui-next with theme support#6008
component(ContextMenuViewport): move to ui-next with theme support#6008dan-rukas wants to merge 7 commits into
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@claude review |
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
Thanks for this. One small comment.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a new ChangesContextMenuViewport Component and Customization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@platform/ui-next/src/components/ContextMenuViewport/ContextMenuViewport.tsx`:
- Around line 27-32: The context menu actions in ContextMenuViewport are
rendered as non-interactive divs, so they cannot be reached or activated with
the keyboard. Update the item rendering in ContextMenuViewport to use a semantic
button-like control for each action, preserve the existing item.action(item,
props) click behavior, and make sure the interactive styling uses cursor-pointer
instead of cursor-default. Ensure the unique item row wrapper and its onClick
handler remain tied to the same ContextMenuViewport mapping logic.
- Around line 16-18: The ContextMenuViewport rendering guard only checks for
falsy items, so an empty array still creates a blank menu container. Update the
conditional in ContextMenuViewport to also skip rendering when the items
collection is empty, using the existing items check before the menu shell is
returned. Keep the fix localized to ContextMenuViewport so empty menus do not
render at all.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cdcef4a-0595-4903-b86c-8ec42560a877
📒 Files selected for processing (4)
extensions/default/src/customizations/contextMenuUICustomization.tsplatform/ui-next/src/components/ContextMenuViewport/ContextMenuViewport.tsxplatform/ui-next/src/components/ContextMenuViewport/index.tsplatform/ui-next/src/components/index.ts
| if (!items) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid rendering an empty menu container when items is empty.
Line 16 only checks for falsy values; [] still renders a blank menu shell.
Suggested fix
- if (!items) {
+ if (!items?.length) {
return null;
}📝 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.
| if (!items) { | |
| return null; | |
| } | |
| if (!items?.length) { | |
| return null; | |
| } |
🤖 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 `@platform/ui-next/src/components/ContextMenuViewport/ContextMenuViewport.tsx`
around lines 16 - 18, The ContextMenuViewport rendering guard only checks for
falsy items, so an empty array still creates a blank menu container. Update the
conditional in ContextMenuViewport to also skip rendering when the items
collection is empty, using the existing items check before the menu shell is
returned. Keep the fix localized to ContextMenuViewport so empty menus do not
render at all.
| <div | ||
| key={index} | ||
| data-cy="context-menu-item" | ||
| onClick={() => item.action(item, props)} | ||
| className="hover:bg-accent hover:text-accent-foreground flex cursor-default select-none items-center justify-between rounded-sm px-2 py-1.5 text-base outline-none" | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use keyboard-focusable controls for menu actions.
Line 27 renders clickable rows as <div>, so actions are not keyboard-focusable/activatable. Line 31 also uses cursor-default on interactive elements. Use a semantic <button> (or equivalent keyboard handlers) and cursor-pointer.
Suggested fix
- <div
+ <button
+ type="button"
key={index}
data-cy="context-menu-item"
onClick={() => item.action(item, props)}
- className="hover:bg-accent hover:text-accent-foreground flex cursor-default select-none items-center justify-between rounded-sm px-2 py-1.5 text-base outline-none"
+ className="hover:bg-accent hover:text-accent-foreground flex w-full cursor-pointer select-none items-center justify-between rounded-sm px-2 py-1.5 text-base outline-none"
>
<span>{item.label}</span>
{item.iconRight && (
<Icons.ByName
name={item.iconRight}
className="inline"
/>
)}
- </div>
+ </button>📝 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.
| <div | |
| key={index} | |
| data-cy="context-menu-item" | |
| onClick={() => item.action(item, props)} | |
| className="hover:bg-accent hover:text-accent-foreground flex cursor-default select-none items-center justify-between rounded-sm px-2 py-1.5 text-base outline-none" | |
| > | |
| <button | |
| type="button" | |
| key={index} | |
| data-cy="context-menu-item" | |
| onClick={() => item.action(item, props)} | |
| className="hover:bg-accent hover:text-accent-foreground flex w-full cursor-pointer select-none items-center justify-between rounded-sm px-2 py-1.5 text-base outline-none" | |
| > |
🤖 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 `@platform/ui-next/src/components/ContextMenuViewport/ContextMenuViewport.tsx`
around lines 27 - 32, The context menu actions in ContextMenuViewport are
rendered as non-interactive divs, so they cannot be reached or activated with
the keyboard. Update the item rendering in ContextMenuViewport to use a semantic
button-like control for each action, preserve the existing item.action(item,
props) click behavior, and make sure the interactive styling uses cursor-pointer
instead of cursor-default. Ensure the unique item row wrapper and its onClick
handler remain tied to the same ContextMenuViewport mapping logic.
2 participants
Context
Moves the viewport context menu from
@ohif/uito@ohif/ui-nextasContextMenuViewport. The legacy component used hardcoded colors that don't respond to theming. The new component uses ui-next token-based styling and includes minor UX improvements.Changes & Results
ContextMenuViewportto@ohif/ui-next— same props API as the legacy component, now with theme-aware styling and UX polishcontextMenuUICustomization.tsto import from@ohif/ui-nextTesting
Checklist
PR
semantic-release format and guidelines.
Suggested PR title:
feat(ui-next): add ContextMenuViewport component with theme supportCode
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
Migrates the viewport context menu from
@ohif/ui(ContextMenu) to@ohif/ui-next(ContextMenuViewport), replacing hardcoded color classes with Tailwind token-based theming and adding a subtle open animation. The props contract and item-action wiring are preserved, so existing controller and menu-item builder code works without changes.ContextMenuViewportis functionally equivalent to the old component;item.action(item, props)still has access toonClose,onShowSubMenu, andonDefaultvia the...propsspread, so menu close-on-click and submenu navigation continue to work.cursor-default select-nonereplacescursor-pointeron interactive items — this matches shadcn/ui conventions but removes the pointer-cursor affordance users typically expect from clickable menu rows.defaultPositionis declared in the TypeScript interface but is never read by the component (positioning is managed externally by the dialog service).Confidence Score: 4/5
Safe to merge; the migration is behaviorally equivalent and all observations are minor style and polish items.
The component faithfully mirrors the old ContextMenu's prop wiring and close-on-click behavior. The only tangible change worth a second look is the switch from
cursor-pointertocursor-defaulton menu items, which removes a standard interactive affordance without a clear design reason.ContextMenuViewport.tsx — the cursor class and unused interface prop are both worth a quick look before merge.
Important Files Changed
cursor-defaultinstead ofcursor-pointeron interactive items and declares an unuseddefaultPositionprop in the interface.@ohif/uiContextMenu to@ohif/ui-nextContextMenuViewport; straightforward and correct.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Refine styling" | Re-trigger Greptile
Summary by CodeRabbit