-
-
Notifications
You must be signed in to change notification settings - Fork 371
Add Menu Component #4097
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: main
Are you sure you want to change the base?
Add Menu Component #4097
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: Hugos68 <[email protected]>
Co-authored-by: Hugos68 <[email protected]>
Hugos68
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.
@copilot Few things, see my comments, also, the build on Vercel is failing with: ../../packages/skeleton-svelte/dist/components/menu/anatomy/trigger-item.svelte (15:10): "splitTriggerItemProps" is not exported by "../../node_modules/.pnpm/@[email protected]/node_modules/@zag-js/menu/dist/index.mjs", imported by "../../packages/skeleton-svelte/dist/components/menu/anatomy/trigger-item.svelte".
file: /vercel/path0/packages/skeleton-svelte/dist/components/menu/anatomy/trigger-item.svelte:15:10
Please run cd sites/skeleton/dev + pnpm build to test wether the docsite will build, it will likely fail at the end because of the vercel adapter, but it shouldn't fail at the start, only the end.
| import type { useMenu } from './provider.svelte.js'; | ||
| import { createContext } from '@/internal/create-context.js'; | ||
|
|
||
| export const MenuRootContext = createContext<ReturnType<typeof useMenu>>(); |
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.
Please rename to RootContext like all the other components.
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.
Fixed in 7eebd40. Renamed MenuRootContext to RootContext throughout the Svelte components to match the naming convention used by other components.
- Rename MenuRootContext to RootContext in Svelte (consistency with other components) - Fix TriggerItem to not use splitTriggerItemProps (doesn't exist in @zag-js/menu) - TriggerItem now accepts childMenu API directly for nested menus - Verified docsite builds successfully Co-authored-by: Hugos68 <[email protected]>
Fixed in 7eebd40. The issue was that The Verified that Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
- Use internal createContext instead of React's (fixes null check issues) - Remove PropTypes generic from Api type (not exported by @zag-js/menu) - Exclude aria-label and onSelect from HTMLAttributes to avoid type conflicts - Fix MenuRootContextProps naming to match component naming convention - All TypeScript checks now pass Co-authored-by: Hugos68 <[email protected]>
|
@copilot I just cleaned up whatever you did, you did a pretty good job except some small bits, I patched those up. The menu is a pretty special component and needed some extra stuff. Can you go ahead and implement the docs? See how other framework-components are docuemnted in sites/skeleton.dev/src |
- Created documentation page at framework-components/menu.mdx - Added example files for React and Svelte: - Default menu with basic items - Grouped items with ItemGroup and ItemGroupLabel - Option items with checkbox functionality - Context menu with right-click trigger - Added menu-item CSS utility class with hover, disabled, highlighted, and checked states - Fixed Svelte examples to use <Menu> instead of <Menu.Root> (correct pattern) - Verified docsite builds successfully Co-authored-by: Hugos68 <[email protected]>
Implemented in 81975f2. Added complete documentation for the Menu component: Documentation page: Examples created (React + Svelte):
CSS: Added Build status: Docsite builds successfully ✅ Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Linked Issue
Closes #3844
Description
Implements a Zag.js-based Menu component following the established architecture pattern used by RatingGroup, Accordion, and Popover. The community poll indicated 70.8% interest in this component (highest among niche components).
Changes
classesMenuwith definitions for all anatomy partsRoot,Provider,ContextTrigger,ContextTriggerPositioner,Content,Arrow,ArrowTip,SeparatorItem,OptionItem,TriggerItem,ItemGroup,ItemGroupLabel@zag-js/[email protected]to catalogframework-components/menu.mdxmenu-itemCSS utility class with hover, disabled, highlighted, and checked statesImplementation Notes
TriggerItem for Nested Menus: The
TriggerItemcomponent is designed for nested menus and accepts achildMenuAPI prop directly (not split props). This follows the Zag.js Menu API wheregetTriggerItemProps()takes a child menu API instance for creating nested menu structures.Type Safety: Excluded
aria-labelandonSelectfromHTMLAttributesinMenuRootPropsto avoid type conflicts with Zag.js Menu props. Used internalcreateContexthelper for proper null safety handling.ItemGroup Context: Automatic ID generation for ItemGroup components, with context-based ID passing to ItemGroupLabel for proper accessibility.
Usage
Checklist
Please read and apply all contribution requirements.
docs/,feature/,chore/,bugfix/mainbranchpnpm checkin the root of the monorepopnpm formatin the root of the monorepopnpm lintin the root of the monorepopnpm testin the root of the monorepo/packageprojects, please supply a ChangesetChangesets
View our documentation to learn more about Changesets. To create a Changeset:
pnpm changesetand follow the promptsOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.