Skip to content

Conversation

ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Apr 22, 2025

Description

This PR enhances the Menu's Item component by adding support for navigational links.

Details

  • Added two optional props to Menu Item:
    • href: When provided, the menu item is rendered as an anchor.
    • isExternal: When true, adds appropriate attributes (target="_blank" and rel="noopener noreferrer") to external links.
  • Ensures compliance with the ARIA Menu Button Link pattern

Screenshot 2025-04-21 at 2 53 34 PM
Screenshot 2025-04-21 at 2 53 22 PM

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • ⚫ renders as expected in dark mode
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

geotrev and others added 5 commits June 27, 2024 11:58
… george/menu-links

# Conflicts:
#	packages/dropdowns/src/elements/menu/Item.tsx
#	packages/dropdowns/src/views/combobox/StyledOption.ts
#	packages/dropdowns/src/views/menu/StyledItem.ts
#	packages/dropdowns/src/views/menu/StyledItemContent.ts
#	packages/dropdowns/src/views/menu/StyledItemIcon.ts
isSelected,
icon,
isDisabled,
isExternal = true,
Copy link
Member

Choose a reason for hiding this comment

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

In keeping with the majority of Garden API, this should not default to true. Let's let devs be specific about this.

icon?: ReactElement;
/** Indicates that the item is not interactive */
isDisabled?: boolean;
/** If the item is an anchor, opens the link externally */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** If the item is an anchor, opens the link externally */
/** Opens the `href` externally */

Follow documentation grammar guidelines.

as: 'a'
})`
direction: ${props => props.theme.rtl && 'rtl'};
color: ${({ theme }) => getColor({ theme, variable: 'foreground.primary' })};
Copy link
Member

Choose a reason for hiding this comment

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

At this point, there should be no visual differentiation between <Item> and <Item href>. So the styling here should be about resetting any potential <a> styling (color, underline, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying. I followed the design from George's branch but I'll update.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Have we thought through what it means for these new <Item href> to be a part of a <ItemGroup type="radio">? What are the implications for existing uncontrolled behaviors?

@ze-flo
Copy link
Contributor Author

ze-flo commented May 14, 2025

Closing in favor of #2021.

@ze-flo ze-flo closed this May 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants