Skip to content

feat(menu, menu-item): add scales, update padding, spacing, font-sizes#14797

Draft
Elijbet wants to merge 4 commits into
devfrom
elijbet/12337-menu-menu-item-scales-padding-spacing-font-size
Draft

feat(menu, menu-item): add scales, update padding, spacing, font-sizes#14797
Elijbet wants to merge 4 commits into
devfrom
elijbet/12337-menu-menu-item-scales-padding-spacing-font-size

Conversation

@Elijbet

@Elijbet Elijbet commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Related Issue: #12337

Summary

Add menu and menu-item scales, update padding, spacing, font-sizes.

@github-actions github-actions Bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jul 10, 2026
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jul 10, 2026
@Elijbet
Elijbet marked this pull request as ready for review July 10, 2026 20:01
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jul 10, 2026
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jul 10, 2026

@jcfranco jcfranco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good, @Elijbet!

@brendan-vincent-rice @ashetland Could you take a look at this one? 🙇‍♂️


:host([layout="vertical"]) .icon--end ~ .icon--breadcrumb {
@apply ms-2;
margin-inline-start: 0.5rem;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should use --calcite-space-sm.

}

:host([slot="submenu-item"][scale="l"][layout="vertical"]) {
min-block-size: theme("spacing.16");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are there any token equivalents for these? We need to avoid additional Tailwind usage (see #12080). Applies to other new styles.


//#region Private Methods

private getContentIconScale(): "s" | "m" {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not use getIconScale directly? 🤔

}
}

.content-container {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you check if this is always overridden? If so, the default styles (e.g., flex items-center) could be moved here.

}

.icon--dropdown {
box-sizing: border-box;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could these styles be DRYed up a bit by using additional internal custom CSS props?

></calcite-menu-item>
</calcite-menu>`;

export const scaleComparison = (): string => html`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For consistency, can you use allScales for the story name?

@jcfranco

Copy link
Copy Markdown
Member

Also, should this use feat as it's adding support for scales and updating sizing?

@Elijbet Elijbet changed the title fix(menu, menu-item): add scales, update padding, spacing, font-sizes feat(menu, menu-item): add scales, update padding, spacing, font-sizes Jul 14, 2026
@Elijbet
Elijbet marked this pull request as draft July 14, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants