Skip to content

style(menu): conform to code style guides #6799

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.

Sign up for GitHub

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

Merged
Rajdeepc merged 2 commits into from
Sep 30, 2026
+92 −117
Merged

style(menu): conform to code style guides #6799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41bbf60
style(menu): conform to TypeScript, test, and Storybook style guides
Sep 29, 2026
0094699
style(menu): address review feedback on the conformance pass
Sep 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions 2nd-gen/packages/swc/components/menu/Menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,18 @@ import { MenuBase } from '@adobe/spectrum-wc-core/components/menu';
import styles from './menu.css';

/**
* A menu is a full menu-button host: an externally-referenced trigger opens a
* `PlacementController`-anchored surface containing a `role="menu"` list of
* `swc-menu-item` rows (`swc-menu-group` and `swc-divider` as a separator
* join in a later migration phase).
* A menu is a list of actions or options anchored to an externally-referenced
* trigger, following the menu-button pattern. `swc-menu-item` rows
* (`swc-menu-group` and `swc-divider` as a separator join in a later
* migration phase) render in a shadow-internal `role="menu"` surface.
*
* The behavior (trigger and ARIA wiring, positioning, keyboard navigation,
* and event dispatch) lives in `MenuBase`; this class supplies only the

@Rajdeepc Rajdeepc Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review — checked against Popover.ts precedent: this reads correct.

Dropping PlacementController here is right: it's an internal collaborator, and this JSDoc is CEM-facing (it feeds <ApiTable /> and the docs page). Popover.ts likewise never names it.

I did stop to question whether naming MenuBase just swaps one internal for another, but Popover.ts:25-27 says almost exactly the same thing:

The behavior (lifecycle, trigger and ARIA wiring, positioning, events) lives in PopoverBase; this class supplies only the styles, the render template, and the shadow-DOM element getters the base reads.

So the base class is an established, documented architectural concept in this JSDoc slot, unlike a collaborator. Deliberately mirrored.

One gap this PR arguably should have caught: Menu.ts has no @since tag, while Popover.ts, Tooltip.ts, and Badge.ts all carry one (Popover.ts has @since 2.0.0-beta.2 right under @element). It isn't mandated by 07_jsdoc-standards.md, so I left it rather than invent a version — but if the 3-of-3 peer convention is intentional, this block is the place to add it and I'm happy to. Flagging rather than guessing the value.

@caseyisonit caseyisonit Sep 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think the since tag should be added since all other components have it. it should be +1 version to whatever the next release will be

* styles, the render template, and the shadow-DOM element getter it
* overrides.
*
* @element swc-menu
* @since 2.0.0-beta.4
*
* @example
* <swc-menu>
Expand Down
7 changes: 6 additions & 1 deletion 2nd-gen/packages/swc/components/menu/stories/menu.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ const meta: Meta = {
export default meta;

// ────────────────────
// PLAYGROUND STORY
// HELPERS
// ────────────────────

// Placeholders for `swc-menu-item`, which doesn't exist yet; matches its
Expand All @@ -84,6 +84,10 @@ const defaultItems = html`
<swc-menu-item role="menuitem" tabindex="-1">Paste</swc-menu-item>
`;

// ────────────────────
// PLAYGROUND STORY
// ────────────────────

export const Playground: Story = {
args: {
open: false,
Expand Down Expand Up @@ -272,6 +276,7 @@ export const TriggerElement: Story = {
`;
},
};
TriggerElement.storyName = 'Trigger element';

@Rajdeepc Rajdeepc Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review

  1. storyName coverage is now complete, not partial. Only two exports in this file are multi-word (OpenAndClose, TriggerElement), and both now have an override. The other seven (Playground, Overview, Anatomy, Sizes, Placements, States, Accessibility) are single-word, where Storybook's default PascalCase to Title Case conversion already yields sentence case, so they correctly need nothing.

  2. The heading it has to match actually exists. menu.mdx line 48 is ### Trigger element; without this override Storybook rendered "Trigger Element", so the MDX heading and the rendered story title disagreed. popover.stories.ts:265 and tooltip.stories.ts:517 both set the identical override on their own TriggerElement story.

The HELPERS separator move above matches the section order in stories-format.md (METADATA → HELPERS → PLAYGROUND STORY) and popover.stories.ts's own layout, where shared story content (accountCard, securityPanel) sits in HELPERS. defaultItems is consumed by 7 stories, so it belongs there rather than under PLAYGROUND STORY.


// ────────────────────────────────
// ACCESSIBILITY STORIES
Expand Down
Loading
Loading