feat(evo-react): add fake menu - #790
Conversation
🦋 Changeset detectedLatest commit: 4b23dfa The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new EvoFakeMenu compound-component API to @evo-web/react, mirroring the Skin/legacy fake-menu markup patterns while providing React 19-friendly composition, SSR/browser tests, Storybook docs, and app migration guidance.
Changes:
- Introduces
EvoFakeMenu+ subcomponents (Items,Item,ItemBadge,Separator) with shared context and Skin backing styles. - Adds Vitest browser + SSR snapshot coverage and a Storybook docs story for the new component.
- Updates the app-migration skill docs and adds a patch changeset for
@evo-web/react.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/evo-react/src/fake-menu/types.ts | Public prop types for the compound API (root/items/item/badge/separator). |
| packages/evo-react/src/fake-menu/context.tsx | Context plumbing for prefixed BEM base class. |
| packages/evo-react/src/fake-menu/fake-menu.tsx | Root wrapper applying Skin classes/modifiers and providing context. |
| packages/evo-react/src/fake-menu/fake-menu-items.tsx | <ul> wrapper establishing fake-menu__items + tabIndex={-1}. |
| packages/evo-react/src/fake-menu/fake-menu-item.tsx | Anchor/button item rendering, current state, disabled handling, tick icon. |
| packages/evo-react/src/fake-menu/fake-menu-item-badge.tsx | Badge wrapper mapping to EvoBadge with type="menu". |
| packages/evo-react/src/fake-menu/fake-menu-separator.tsx | Separator <hr> inside <li> with context-prefixed separator class. |
| packages/evo-react/src/fake-menu/index.ts | Barrel exports for component entrypoint + types. |
| packages/evo-react/src/fake-menu/README.md | Minimal component README pointing to Storybook docs. |
| packages/evo-react/src/fake-menu/fake-menu.stories.tsx | Storybook “autodocs” story + custom link adapter example. |
| packages/evo-react/src/fake-menu/test/test.server.tsx | SSR snapshot coverage for key variants and classPrefix modifiers. |
| packages/evo-react/src/fake-menu/test/test.browser.tsx | Browser tests for roles/attrs, current state, refs, and disabled behavior. |
| packages/evo-react/src/fake-menu/test/snapshots/test.server.tsx.snap | Stored SSR snapshots for the new component. |
| .claude/skills/evo-app-migrate-react/SKILL.md | Adds evo-fake-menu to the supported migration mapping table. |
| .claude/skills/evo-app-migrate-react/components/evo-fake-menu.md | Component-specific migration guidance for apps moving off ebay-fake-menu. |
| .changeset/add-evo-fake-menu.md | Patch changeset announcing the new component. |
| <Component | ||
| {...(rest as ComponentProps<"a">)} | ||
| href={disabled ? undefined : href} | ||
| aria-disabled={disabled ? "true" : undefined} | ||
| aria-current={ariaCurrent} | ||
| className={itemClassName} | ||
| > |
| import type { EvoBadgeProps } from "../badge/types"; | ||
|
|
||
| export type EvoFakeMenuProps = ComponentProps<"span"> & { | ||
| /** Prefix applied to item and separator BEM classes when embedded in another component. */ |
PR Preview DeployedWebsite • commit 4b23dfa |
ArtBlue
left a comment
ArtBlue
left a comment
There was a problem hiding this comment.
Solid compound-component structure and thorough test/story coverage overall, but there's one blocking bug: EvoFakeMenuItem never applies the fake-menu__item--badged modifier class Skin's CSS requires for badge positioning, and nests the badge inside the label <span> instead of rendering it as a sibling — confirmed in the SSR snapshot (<span>Messages<span class="badge...">5</span></span>). The legacy ebayui-core-react implementation gets this right (hasBadge && "menu__item--badged"); this compound-API redesign lost it. Also non-blocking: the separator drops the explicit role="separator" its Marko sibling sets (native <hr> implies it, so low risk, but EvoFakeMenuSeparatorProps explicitly omits role from the props type, implying the intent was to hardcode it). Separately, both this PR's description and the migration doc say EvoFakeMenu follows "the same compound-component API as EvoMenu", but no EvoMenu React component exists in this repo (only evo-marko's evo-menu, which uses a different attr-tag/slot API) — worth double-checking that reference isn't a leftover from a different draft.
| export function EvoFakeMenuSeparator({ | ||
| className, | ||
| ...rest | ||
| }: EvoFakeMenuSeparatorProps) { |
There was a problem hiding this comment.
Non-blocking: EvoFakeMenuSeparatorProps omits role from the native <hr> props (implying the component controls it), but no role="separator" is ever set here — the evo-marko sibling sets it explicitly (<hr class=... role="separator">). Native <hr> does default to an implicit separator role in most browsers/AT, so this likely isn't a real regression today, but consider setting it explicitly to match the sibling implementation and the apparent intent behind omitting role from the prop type.
There was a problem hiding this comment.
There is a jsx-a11y/no-redundant-roles in eslint that fails if we add role="separator"
# Conflicts: # .claude/skills/evo-app-migrate-react/SKILL.md # .claude/skills/evo-migrate-react/SKILL.md
Description
Adds
EvoFakeMenuto@evo-web/reactusing the explicit compound-component API established byEvoMenu.as, current and disabled states, class prefixes, and positioning variants.:has(.badge)while retaining the legacy--badgedselectors for compatibility.Notes
No issue number was provided. No verification hooks were bypassed.
Screenshots
Percy dry-run discovery found all eight
Skin/Menu/Fake Menustories, including the badged use case. No Percy build was created in dry-run mode.Checklist