-
Notifications
You must be signed in to change notification settings - Fork 638
ActionBar: Add support for groups #6979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 8eed237 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces group functionality to the ActionBar component by adding a new ActionBar.Group
sub-component. This enhancement allows related actions to be logically grouped together and appear as submenus in the overflow menu when space is limited.
Key changes:
- Adds
ActionBar.Group
component for organizing related actions - Introduces submenu functionality within the overflow menu for grouped items
- Updates overflow behavior to handle groups as units rather than individual items
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/react/src/ActionBar/index.ts |
Export new ActionBarGroup component and add it to ActionBar compound component |
packages/react/src/ActionBar/ActionBar.tsx |
Implement ActionBarGroup component and update overflow logic to support grouped items |
packages/react/src/ActionBar/ActionBar.test.tsx |
Add tests for group ordering and conditional rendering with groups |
packages/react/src/ActionBar/ActionBar.module.css |
Add CSS styling for the Group component |
packages/react/src/ActionBar/ActionBar.examples.stories.tsx |
Add story example demonstrating ActionBar.Group usage |
packages/react/src/ActionBar/ActionBar.docs.json |
Document ActionBar.Group component props |
.changeset/busy-islands-fail.md |
Add changeset entry for minor release |
disabled: !!disabled, | ||
onClick: onClick as MouseEventHandler, | ||
width: widthRef.current, | ||
groupId: groupId ?? undefined, // todo: remove conditional |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the unnecessary conditional and TODO comment. The groupId ?? undefined
expression is redundant since groupId
is already string | null
and can be passed directly.
groupId: groupId ?? undefined, // todo: remove conditional | |
groupId: groupId, |
Copilot uses AI. Check for mistakes.
// TODO: refine this so that we don't have to loop through the registry multiple times | ||
const groupedItems = Array.from(childRegistry).filter(([, childProps]) => { | ||
if (childProps?.type !== 'action') return false | ||
if (childProps.groupId !== id) return false | ||
return true | ||
}) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
return () => { | ||
unregisterChild(id) | ||
} |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The cleanup function can be simplified to return () => unregisterChild(id)
since it's just a single function call.
return () => { | |
unregisterChild(id) | |
} | |
return () => unregisterChild(id) |
Copilot uses AI. Check for mistakes.
@TylerJDev @pksjce I just want to make sure this is captured correctly. The use of For Groups, I was pitching that it made sense for groups of items to move into the menu in certain cases (like when you have bold, italics, and underline) because leaving only one or two of the options didn't really make sense. For sub-menus, this was meant for a continuation of the action menu components in the action bar itself. For instance, the Heading button would have a dropdown with headings 1-6. When that overflows into the menu, that item would then need to have a sub-menu. Hope that clarifies things 😄 |
Closes https://github.com/github/primer/issues/5928
Adds sub component
ActionBar.Group
.ActionBar.Group
is usedChangelog
New
ActionBar.Group
sub componentRollout strategy
Testing & Reviewing
Merge checklist