-
Notifications
You must be signed in to change notification settings - Fork 670
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
Changes from 7 commits
85ccd4d
2b1871d
f8e21be
dbcc75b
b2bff28
20dd785
29f7f97
8eed237
7538ee4
87cb0c3
cbd06b7
4bbe643
8035d0a
f1bf903
b0a816e
77076a3
740ee43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@primer/react': minor | ||
| --- | ||
|
|
||
| ActionBar: Adds `ActionBar.Group` sub component |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,3 +33,7 @@ | |
| background: var(--borderColor-muted); | ||
| } | ||
| } | ||
|
|
||
| .Group { | ||
| display: flex; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,8 +26,11 @@ type ChildProps = | |||||||||
| icon: ActionBarIconButtonProps['icon'] | ||||||||||
| onClick: MouseEventHandler | ||||||||||
| width: number | ||||||||||
| groupId?: string | ||||||||||
| groupLabel?: string | ||||||||||
| } | ||||||||||
| | {type: 'divider'; width: number} | ||||||||||
| | {type: 'group'; width: number; label: string} | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Registry of descendants to render in the list or menu. To preserve insertion order across updates, children are | ||||||||||
|
|
@@ -38,9 +41,18 @@ type ChildRegistry = ReadonlyMap<string, ChildProps | null> | |||||||||
| const ActionBarContext = React.createContext<{ | ||||||||||
| size: Size | ||||||||||
| registerChild: (id: string, props: ChildProps) => void | ||||||||||
| unregisterChild: (id: string) => void | ||||||||||
| unregisterChild: (id: string, groupId?: string) => void | ||||||||||
| isVisibleChild: (id: string) => boolean | ||||||||||
| }>({size: 'medium', registerChild: () => {}, unregisterChild: () => {}, isVisibleChild: () => true}) | ||||||||||
| groupId?: string | ||||||||||
| groupLabel?: string | ||||||||||
| }>({ | ||||||||||
| size: 'medium', | ||||||||||
| registerChild: () => {}, | ||||||||||
| unregisterChild: () => {}, | ||||||||||
| isVisibleChild: () => true, | ||||||||||
| groupId: undefined, | ||||||||||
| groupLabel: undefined, | ||||||||||
| }) | ||||||||||
|
|
||||||||||
| /* | ||||||||||
| small (28px), medium (32px), large (40px) | ||||||||||
|
|
@@ -107,7 +119,10 @@ const getMenuItems = ( | |||||||||
| childRegistry: ChildRegistry, | ||||||||||
| hasActiveMenu: boolean, | ||||||||||
| ): Set<string> | void => { | ||||||||||
| const registryEntries = Array.from(childRegistry).filter((entry): entry is [string, ChildProps] => entry[1] !== null) | ||||||||||
| const registryEntries = Array.from(childRegistry).filter( | ||||||||||
| (entry): entry is [string, ChildProps] => | ||||||||||
| entry[1] !== null && (entry[1].type !== 'action' || entry[1].groupId === undefined), | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| if (registryEntries.length === 0) return new Set() | ||||||||||
| const numberOfItemsPossible = calculatePossibleItems(registryEntries, navWidth) | ||||||||||
|
|
@@ -234,11 +249,11 @@ export const ActionBar: React.FC<React.PropsWithChildren<ActionBarProps>> = prop | |||||||||
|
|
||||||||||
| if (menuItem.type === 'divider') { | ||||||||||
| return <ActionList.Divider key={id} /> | ||||||||||
| } else { | ||||||||||
| } else if (menuItem.type === 'action' && !menuItem.groupLabel) { | ||||||||||
| const {onClick, icon: Icon, label, disabled} = menuItem | ||||||||||
| return ( | ||||||||||
| <ActionList.Item | ||||||||||
| key={label} | ||||||||||
| key={id} | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was flagged earlier that it could change. Hence we'd kept it as
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha! Added it back! |
||||||||||
| // eslint-disable-next-line primer-react/prefer-action-list-item-onselect | ||||||||||
| onClick={(event: React.MouseEvent<HTMLLIElement, MouseEvent>) => { | ||||||||||
| closeOverlay() | ||||||||||
|
|
@@ -254,6 +269,49 @@ export const ActionBar: React.FC<React.PropsWithChildren<ActionBarProps>> = prop | |||||||||
| </ActionList.Item> | ||||||||||
| ) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // 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.
Sorry, something went wrong. |
||||||||||
|
|
||||||||||
| if (menuItem.type === 'group') { | ||||||||||
| return ( | ||||||||||
| <ActionMenu key={id}> | ||||||||||
| <ActionMenu.Anchor> | ||||||||||
| <ActionList.Item>{menuItem.label}</ActionList.Item> | ||||||||||
| </ActionMenu.Anchor> | ||||||||||
| <ActionMenu.Overlay> | ||||||||||
| <ActionList> | ||||||||||
| {groupedItems.map(([key, childProps]) => { | ||||||||||
| if (childProps && childProps.type === 'action') { | ||||||||||
| const {onClick, icon: Icon, label, disabled} = childProps | ||||||||||
| return ( | ||||||||||
| <ActionList.Item | ||||||||||
| key={key} | ||||||||||
| onSelect={event => { | ||||||||||
| closeOverlay() | ||||||||||
| focusOnMoreMenuBtn() | ||||||||||
| typeof onClick === 'function' && onClick(event as React.MouseEvent<HTMLElement>) | ||||||||||
| }} | ||||||||||
| disabled={disabled} | ||||||||||
| > | ||||||||||
| <ActionList.LeadingVisual> | ||||||||||
| <Icon /> | ||||||||||
| </ActionList.LeadingVisual> | ||||||||||
| {label} | ||||||||||
| </ActionList.Item> | ||||||||||
| ) | ||||||||||
| } | ||||||||||
| return null | ||||||||||
| })} | ||||||||||
| </ActionList> | ||||||||||
| </ActionMenu.Overlay> | ||||||||||
| </ActionMenu> | ||||||||||
| ) | ||||||||||
| } | ||||||||||
| })} | ||||||||||
| </ActionList> | ||||||||||
| </ActionMenu.Overlay> | ||||||||||
|
|
@@ -272,6 +330,7 @@ export const ActionBarIconButton = forwardRef( | |||||||||
| const id = useId() | ||||||||||
|
|
||||||||||
| const {size, registerChild, unregisterChild, isVisibleChild} = React.useContext(ActionBarContext) | ||||||||||
| const {groupId} = React.useContext(ActionBarGroupContext) | ||||||||||
|
|
||||||||||
| // Storing the width in a ref ensures we don't forget about it when not visible | ||||||||||
| const widthRef = useRef<number>() | ||||||||||
|
|
@@ -288,9 +347,12 @@ export const ActionBarIconButton = forwardRef( | |||||||||
| disabled: !!disabled, | ||||||||||
| onClick: onClick as MouseEventHandler, | ||||||||||
| width: widthRef.current, | ||||||||||
| groupId: groupId ?? undefined, // todo: remove conditional | ||||||||||
|
||||||||||
| groupId: groupId ?? undefined, // todo: remove conditional | |
| groupId: groupId, |
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) |
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.
Do we need this
groupLabel? I'm not sure if its used anywhereThere 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.
Good catch, we don't, I removed it!