ActionMenu: Only use checkmarks in menus and improve examples

[maximedegreve]

Closes #3878

Changelog

• Always use checkmarks for menus instead of checkboxes
• Improve the examples

Before / After

Removed

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• Integration tests pass at github/github (Learn more about how to run integration tests)

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: f117b6b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	104.75 KB (+0.01% 🔺)
dist/browser.umd.js	105.31 KB (+0.01% 🔺)

[maximedegreve]

I utilized this search query to determine the effects of my changes on our GitHub platform.

I'm currently investigating which product areas will be impacted by this change and will outline them below. I've also included some further insights on implementations that are unaffected but have potential for enhancement.

Projects

In roadmap project boards, the "milestones" menu will be updated to use checkmarks instead. This change aligns it with other menus, such as the "date fields" menu. However, there's an issue with consistency in how these menus behave. Some close immediately after a selection is made, while others stay open. Ideally, dropdown menus should close once a selection is made. If they need to stay open, a SelectPanel with a dialog and a close button should be used for better accessibility.

Action Required: While the above issue is not critical and will just cause a visual update, it's important to notify the projects team to address these inconsistencies.

Although my updates won't impact this area, I'd like to mention that I discovered a glitch in the add column feature for boards.

The field sum menu will change to checkmarks instead of checkboxes. If they want to keep the option after making a selection instead of using e.stopPropagation() they should switch to a SelectPanel.

Action Required: The above is not critical since it's only a visual change for them. However the use of e.stopPropagation() in the menu keeps it open after making a selection. This is an accessibility violation.

Pull requests

This experience is currently being developed, so modifying the checkmarks is safe. Importantly, this change will be advantageous for this team, as it will ensure the behavior aligns perfectly with what we aim to address in this PR.

Notification settings

In the notifications, the ActionMenu will change to use checkmarks. However, currently, the menu stays open after a selection is made, which isn't the intended behavior. I've contacted our team to fix this issue. We need to switch to using the SelectPanel instead of the ActionMenu there to solve this problem.

Resolved through despite the tooltip bug. This bug is unrelated and needs to be resolved separately:

• https://github.com/github/github/pull/303043

Conclusion

From the 23 file matches on the GitHub platform only 7 will be affected because they are either wrapped in the ActionMenu component or have applied role=menu on the ActionList parent. It's worth to note that SelectPanel instances don't get affected as they use the listbox role.

Internal only

Affected

• NotificationsSubscriptionsMenu.tsx#L383 (✅ Unsure about this. There are lot of problems there. Internally I reached out to this team. This is non blocking because as it isn't shipped yet.)
• roadmap-marker-menu.tsx#L76 (✅ Impact explained above)
• add-column-menu.tsx#L156 (🐛 Impact explained above)
• MultipleSelect.tsx#L135 (✅ Impact explained above)
• field-sum-menu.tsx#L136 (🐛 Impact explained above)
• FileFilter.tsx#L42 (✅ Impact explained above)

Unaffected

• add-new-column-option.tsx#L183
• SelectTeamsViewerResults.tsx#L43
• LicenseFilterPane.tsx#L64
• LabelsList.tsx#L32
• SelectTeamsQueryLoading.tsx#L19
• LabelPicker.tsx#L452
• UserPicker.tsx#L234
• AssigneePicker.tsx#L561
• ReviewersPicker.tsx#L403
• PullRequestAndBranchPicker.tsx#L401
• CostCenterResourceSelector.tsx#L92
• SelectTeamsOrgResults.tsx#L42
• RepositoryIdTarget.tsx#L107
• PullRequestPicker.tsx#L333

[maximedegreve]

I believe it's wise to wait for the Notifications and Memex team to adjust to the above changes before we proceed with the merge.

cc @joshblack @lesliecdubs

[lesliecdubs]

@joshblack or @siddharthkp, does validating the changes in this PR fall clearly into either of the epics you are directly working on right now given the impact on ActionMenu a11y and potentially SelectPanel work? If so, we could try to add scope to fix dotcom to those epics. If not, we might need to explicitly plan for how to roll this out in dotcom as part of Q3 work.

What do you think?

[siddharthkp]

does validating the changes in this PR fall clearly into either of the epics you are directly working on right now given the impact on ActionMenu a11y and potentially SelectPanel work?

Doesn't fall into my path in SelectPanel, sorry! Happy to look into it outside the epic, of course

[maximedegreve]

@lesliecdubs @joshblack Internally this pull request was merged which unblocks this. Are you both happy to merge this and agree that the above impact is minimal?

[lesliecdubs]

@maximedegreve I'm comfortable with merging if @siddharthkp also approves. Should we verify with @tallys from the design perspective as well?

[siddharthkp]

@maximedegreve: I believe it's wise to wait for the Notifications and Memex team to adjust to the above changes before we proceed with the merge.

Hi 👋 , do you know if the changes you asked for in notifications and memex have been merged?

[maximedegreve]

@siddharthkp Memex is informed about the updates. However, these won't cause any disruptions because they only alter the appearance. Besides, the current menu behavior there is already inconsistent as described above.