Add ActionList.Heading component #3581
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@primer/react': minor | ||
| --- | ||
|
|
||
| ActionList: Add ActionList.Heading component | ||
|
|
||
| <!-- Changed components: ActionList --> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import React, {forwardRef} from 'react' | ||
| import {BetterSystemStyleObject, SxProp, merge} from '../sx' | ||
| import {defaultSxProp} from '../utils/defaultSxProp' | ||
| import {useRefObjectAsForwardedRef} from '../hooks' | ||
| import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' | ||
| import {default as HeadingComponent} from '../Heading' | ||
| import {ListContext} from './List' | ||
| import VisuallyHidden from '../_VisuallyHidden' | ||
|
|
||
| export type ActionListHeadingProps = { | ||
| as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | ||
| visuallyHidden?: boolean | ||
| } & SxProp | ||
|
|
||
| export const Heading = forwardRef( | ||
| ({as: Component = 'h3', children, sx = defaultSxProp, visuallyHidden = false, ...props}, forwardedRef) => { | ||
| const innerRef = React.useRef<HTMLHeadingElement>(null) | ||
| useRefObjectAsForwardedRef(forwardedRef, innerRef) | ||
|
|
||
| const {headingId: headingId, variant: listVariant} = React.useContext(ListContext) | ||
|
|
||
| const styles = { | ||
| marginBottom: 2, | ||
| marginX: listVariant === 'full' ? 2 : 3, | ||
| } | ||
|
|
||
| return ( | ||
| <VisuallyHidden isVisible={!visuallyHidden}> | ||
| <HeadingComponent | ||
| as={Component} | ||
| ref={innerRef} | ||
| // use custom id if it is provided. Otherwise, use the id from the context | ||
| id={props.id ?? headingId} | ||
| sx={merge<BetterSystemStyleObject>(styles, sx)} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </HeadingComponent> | ||
| </VisuallyHidden> | ||
| ) | ||
| }, | ||
| ) as PolymorphicForwardRefComponent<'h3', ActionListHeadingProps> | ||
|
|
||
| Heading.displayName = 'ActionList.Heading' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| import React, {useId} from 'react' | ||
| import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' | ||
| import styled from 'styled-components' | ||
| import sx, {SxProp, merge} from '../sx' | ||
| import {AriaRole} from '../utils/types' | ||
| import {ActionListContainerContext} from './ActionListContainerContext' | ||
| import {defaultSxProp} from '../utils/defaultSxProp' | ||
| import {useSlots} from '../hooks/useSlots' | ||
| import {Heading} from './Heading' | ||
|
|
||
| export type ActionListProps = React.PropsWithChildren<{ | ||
| /** | ||
|
|
@@ -26,7 +28,10 @@ export type ActionListProps = React.PropsWithChildren<{ | |
| }> & | ||
| SxProp | ||
|
|
||
| type ContextProps = Pick<ActionListProps, 'variant' | 'selectionVariant' | 'showDividers' | 'role'> & { | ||
| headingId?: string | ||
| } | ||
|
|
||
| export const ListContext = React.createContext<ContextProps>({}) | ||
|
|
||
| const ListBox = styled.ul<SxProp>(sx) | ||
|
|
@@ -42,32 +47,42 @@ export const List = React.forwardRef<HTMLUListElement, ActionListProps>( | |
| paddingY: variant === 'inset' ? 2 : 0, | ||
| } | ||
|
|
||
| const [slots, childrenWithoutSlots] = useSlots(props.children, { | ||
| heading: Heading, | ||
| }) | ||
|
|
||
|
There was a problem hiding this comment. Would this already work with eslint-plugin-primer-react or do we need to update that as well? There was a problem hiding this comment. I think we would need to make some update on the eslint plugin.
Although, this rule will only prevent folks from using Also we can add There was a problem hiding this comment. Considering action lists in menus shouldn't have a heading (reference), maybe we can enforece this with the eslint rule as well? There was a problem hiding this comment.
Because ActionList.Heading is a new component, we can also use typescript to restrict allowed values for There was a problem hiding this comment.
Good point. We could add an eslint rule for this, however, I'm not sure how effectively we would be able to catch incorrect usage given ActionList does not have to be a direct child of ActionMenu, it can be abstracted in a custom We also have the option of a dev environment runtime warning. Because we know when an ActionList is contained inside ActionMenu, we could use that to throw a warning There was a problem hiding this comment.
That is so true! I missed that 😅 which is already restricted anyway!
I love this! Thanks for the context. I made it All that said, if you think that is unnecessary and very little chance of creating a misleading experience, happy to make it warning as well 😊 |
||
| const headingId = useId() | ||
|
|
||
| /** if list is inside a Menu, it will get a role from the Menu */ | ||
| const { | ||
| listRole, | ||
| listLabelledBy, | ||
| selectionVariant: containerSelectionVariant, // TODO: Remove after DropdownMenu2 deprecation | ||
| } = React.useContext(ActionListContainerContext) | ||
|
|
||
| const ariaLabelledBy = slots.heading ? slots.heading.props.id ?? headingId : listLabelledBy | ||
|
|
||
| return ( | ||
| <ListContext.Provider | ||
| value={{ | ||
| variant, | ||
| selectionVariant: selectionVariant || containerSelectionVariant, | ||
| showDividers, | ||
| role: role || listRole, | ||
| headingId, | ||
| }} | ||
| > | ||
| {slots.heading} | ||
| <ListBox | ||
| sx={merge(styles, sxProp as SxProp)} | ||
| role={role || listRole} | ||
| aria-labelledby={ariaLabelledBy} | ||
| {...props} | ||
| ref={forwardedRef} | ||
| > | ||
| {childrenWithoutSlots} | ||
| </ListBox> | ||
| </ListContext.Provider> | ||
| ) | ||
| }, | ||
| ) as PolymorphicForwardRefComponent<'ul', ActionListProps> | ||
|
|
||
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.
Non blocking: Should there be a default h3 or should we always ask for the heading level by making
asrequired without default?Related work from eslint-plugin-primer-react:
asto be set for<Heading>eslint-plugin-primer-react#57There 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.
Ohh this is great! Yes I agree we should make it required!