search

ni / nimble

The NI Nimble Design System
https://nimble.ni.dev
MIT License
31 stars 8 forks source link

nimble-menu-item checkbox support #2311

Open fredvisser opened 1 month ago

fredvisser commented 1 month ago

💡 New Component

😯 Problem to Solve

The nimble-menu-button is often used to contain check/selectable items, but today we simply insert a check icon into the nimble-menu-item to style it as selected.

In doing so, we neglect the correct ARIA role="menuitemcheckbox" and require our clients to manage this state.

Screenshot 2024-07-30 at 3 14 41 PM

💁 Proposed Solution

The existing FAST component already supports (Supports fast-menu-item elements or elements with a role of 'menuitem', 'menuitemcheckbox', and 'menuitemradio'").

Perhaps we document that role="menuitemcheckbox" is valid for nimble-menu-item, and style the menu-item to automatically add the checkbox icon when the item is checked?

rajsite commented 1 month ago

Supports fast-menu-item elements or elements with a role of 'menuitem', 'menuitemcheckbox', and 'menuitemradio'"

Wild didn't realize that was part of the template. We probably want an HLD. At first glance I'd say having separate elements like nimble-checkbox-menu-item would be more inline with our api as it allows you to:

  1. Only pay the cost of the checkbox / radio assets if you use them
  2. API-wise avoids having users need to config aria values manually
  3. Inline with how we have done other components (like anchor variations of menu-item, tree-item, etc.)

There could be an argument to use what fast gives but my suspicion is it'll be messier to try and share checkbox / radio styles consistently, but maybe not! HLD / prototype would be nice.

jattasNI commented 1 month ago

Some more questions from team discussion:

  1. what automatic work should this do for indenting menu items to reserve space for icons (even non-checkable items that are next to checkable ones)?
  2. how does the client prevent the menu from closing when a checkable item is clicked (this is recommended by ARIA)? This is related to https://github.com/ni/nimble/issues/1157#issuecomment-2113195072 but needs more discussion about the right solution, what should be automatic, what order events should fire, etc