Improve the self-drawn menu bar usability - #699
Conversation
|
@bet4it is attempting to deploy a commit to the murongg's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
murongg
left a comment
There was a problem hiding this comment.
Requesting changes for two user-visible regressions:
-
Fix disabled item styling with Base UI state attributes.
Menu.ItemandMenu.SubmenuTriggerrender as<div>elements by default and expose disabled state throughdata-disabled. The currentdisabled:*Tailwind utilities compile to:disabled, so they do not match these elements. Disabled entries therefore keep full opacity, a pointer cursor, and hover highlighting even though activation is blocked. Please usedata-disabled:*, exclude disabled items from hover styles, and add focused regression coverage. -
Preserve the existing menu UI and contents. This PR should change the interaction mechanism only. Please restore the previous 4px trigger spacing, the heavier
Markralabel (font-[620]withtext-(--text-heading)), a popup positioned directly against the trigger (sideOffset={0}rather than6), and the previous trigger appearance without the new persistent popup-open background. Please also retain the existing Hide Markra, Hide Others, and Show All entries so the visible menu content remains unchanged.
Once these are addressed, the Base UI menubar can provide the desired hover-switch behavior without introducing visual regressions.
541832a to
a7fd8d3
Compare
murongg
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous disabled-state and most UI-parity feedback. Requesting further changes for the remaining issues on a7fd8d34:
-
Finish restoring the previous menu UI. The shared trigger classes still include
text-(--text-primary), while the Markra-specific classes addtext-(--text-heading). Both utilities have equal specificity, and the generated stylesheet emitstext-primarylater, so Markra still resolves to the regular menu color. Please avoid emitting both conflicting color utilities. Also, the previous submenu geometry used a-4pxvertical offset; the new submenu positioner uses the defaultalignOffset={0}, placing submenus 4px lower. Please restore this withalignOffset={-4}. -
Add the claimed hover-switch regression test. The committed
NativeTitleBartests cover click activation and disabled styling, but contain nomouseEnterorpointerEnterassertion for switching between top-level menus. Please add a focused test that opens Markra, hovers File, then verifies that the File menu replaces the Markra menu. This protects the primary behavior introduced by the PR and makes the test-plan claim accurate.
I independently confirmed that hover switching currently works and that the prior disabled-item issue is resolved; the remaining request is UI parity plus durable coverage.
a7fd8d3 to
63b66e8
Compare
|
Thanks for the careful review! Both points are addressed in 63b66e8:
|
Summary
The self-drawn menu bar on Windows/Linux (Markra/File/Edit/Format/View) did not implement standard menubar tracking: after opening a menu, hovering another menu name did not switch to it. This PR replaces the imperative
showContextMenu()-based menu bar with a declarative Base UI<Menubar>, which provides the standard hover-switch behavior natively.Visible menu content, spacing, and popup geometry are kept unchanged from the previous menu bar — only the interaction mechanism and underlying components change.
What changed
AppMenuBar, a small adapter that renders the existingContextMenuEntry[]menu data as Base UIMenu/Menubarcomponents (submenus and separators included).WindowsNativeTitleBarnow rendersAppMenuBarin the Windows app chrome instead of calling the imperativeshowContextMenu(); the menu data builders are unchanged.data-disabledattribute (opacity-45, default cursor, no hover highlight), since these items render as<div>s where the:disabledpseudo-class never matches.Behavior
Test plan
pnpm test—NativeTitleBar.test.tsx(47 tests) passes, including new coverage for hover-switch, disabled-itemdata-disabledstyling, and submenu interactiontsc --noEmitonpackages/apppasses