Skip to content

Add radio menu item#320

Merged
dbkr merged 8 commits into
mainfrom
florianduros/radio-menu-item
May 14, 2025
Merged

Add radio menu item#320
dbkr merged 8 commits into
mainfrom
florianduros/radio-menu-item

Conversation

@florianduros
Copy link
Copy Markdown
Member

@florianduros florianduros commented Feb 21, 2025

Radio menu item in figma

Implementation close to the CheckboxMenuItem

@florianduros florianduros marked this pull request as ready for review February 26, 2025 10:56
@florianduros florianduros requested a review from a team as a code owner February 26, 2025 10:56
@florianduros florianduros requested review from dbkr and t3chguy and removed request for a team February 26, 2025 10:56
@t3chguy
Copy link
Copy Markdown
Member

t3chguy commented Feb 26, 2025

@florianduros any idea why there is no cloudflare preview?

@florianduros
Copy link
Copy Markdown
Member Author

Hmm strange, I'll rebase on main to see if it's triggering a preview

@florianduros florianduros force-pushed the florianduros/radio-menu-item branch from cc1ee91 to fefad03 Compare February 26, 2025 11:14
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Feb 26, 2025

Deploying compound-web with  Cloudflare Pages  Cloudflare Pages

Latest commit: c8edcab
Status: ✅  Deploy successful!
Preview URL: https://e0694410.compound-web.pages.dev
Branch Preview URL: https://florianduros-radio-menu-item.compound-web.pages.dev

View logs


type Props = Omit<
React.ComponentProps<typeof RadioMenuItemComponent>,
"Icon" | "label" | "checked" | "onSelect" | "name"
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Remove "Icon"

@t3chguy
Copy link
Copy Markdown
Member

t3chguy commented Feb 26, 2025

This looks to not follow best wai-aria practices where arrow keys do not change the radio selection, only space/enter does https://ariakit.org/examples/menu-item-radio - this may just be a symptom of the Story. Could you update the story to match the best practice so that we can confirm the component can be used to achieve this

@florianduros
Copy link
Copy Markdown
Member Author

florianduros commented Mar 5, 2025

Catching up after a bit of time.

@t3chguy I'm looking at the wai aria pratices: https://www.w3.org/WAI/ARIA/apg/patterns/radio/

Right Arrow and Down Arrow: move focus to the next radio button in the group, uncheck the previously focused button, and check the newly focused button. If focus is on the last button, focus moves to the first button.

Arrow keys should change the selection.

@t3chguy
Copy link
Copy Markdown
Member

t3chguy commented Mar 6, 2025

@t3chguy I'm looking at the wai aria pratices: w3.org/WAI/ARIA/apg/patterns/radio

but that's wrong - radio != menuitemradio which is the role you're implementing.

@dbkr
Copy link
Copy Markdown
Member

dbkr commented Apr 30, 2025

ftr, https://www.w3.org/WAI/ARIA/apg/patterns/menubar/examples/menubar-editor/ is an example of the menuitemradio best practice (although even that disagrees with https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/menuitemradio_role on whether space should close the menu)

@dbkr
Copy link
Copy Markdown
Member

dbkr commented May 13, 2025

After much anguish, I think this is ready (we didn't need to manually control the keyboard focus at all, radix does it for us as long as we actually wrap the thing in a menu).

@dbkr dbkr merged commit 228b24c into main May 14, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants