-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: support sections and headers in RAC gridlist #8667
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
Build successful! 🎉 |
Build successful! 🎉 |
* See `useGridList` for more details about grid list. | ||
* @param props - Props for the section. | ||
*/ | ||
export function useGridListSection(props: AriaGridListSectionProps): GridListSectionAria { |
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.
pass state and ref, we always seem to regret not passing them and it's breaking to add them later
@@ -245,7 +246,8 @@ function GridListInner<T extends object>({props, collection, gridListRef: ref}: | |||
values={[ | |||
[ListStateContext, state], | |||
[DragAndDropContext, {dragAndDropHooks, dragState, dropState}], | |||
[DropIndicatorContext, {render: GridListDropIndicatorWrapper}] | |||
[DropIndicatorContext, {render: GridListDropIndicatorWrapper}], | |||
[SectionContext, {name: 'GridListSection', render: GridListSectionInner}] |
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.
what's this name for?
|
||
const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { |
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 a special one for GridList? I assume it has to do with the extra row div? would it make more sense as two components instead?
It's a little weird because ordinarily the ref would attach to the outer most element. This also makes it hard to style the div since there's no classname on it
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.
i guess we could do something like
<GridListHeader>
<Header />
</GridListHeader>
where GridListHeader is just a div with the role='row'
but yeah, the reason is again for this was because the role=row-header as to be inside a div with a role=row
|
||
const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { |
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.
with ListBox, you can just use a normal Header
, but with GridList, in order to follow correct aria-pattern, the header must be inside a div with a role=row
, hence why i've created a new component called GridListHeader. are we okay with that?
from WAI-ARIA:
Each cell is either a DOM descendant of or owned by a row element and has one of the following roles:
- columnheader if the cell contains a title or header information for the column.
- rowheader if the cell contains title or header information for the row.
- gridcell if the cell does not contain column or row header information.
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.
I think this is ok, but the row
itself should get aria-attributes like aria-rowindex
when virtualized if so.
|
||
const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { |
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.
i guess we could do something like
<GridListHeader>
<Header />
</GridListHeader>
where GridListHeader is just a div with the role='row'
but yeah, the reason is again for this was because the role=row-header as to be inside a div with a role=row
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:GridListHeader+GridListHeader {
+ UNTYPED
+} /react-aria-components:GridListSection+GridListSection <T extends {}> {
+ aria-label?: string
+ children?: ReactNode | ({}) => ReactElement
+ className?: string
+ dependencies?: ReadonlyArray<any>
+ id?: Key
+ items?: Iterable<{}>
+ style?: CSSProperties
+ value?: {}
+} @react-aria/gridlist/@react-aria/gridlist:useGridListSection+useGridListSection {
+ props: AriaGridListSectionProps
+ returnVal: undefined
+} @react-aria/i18n/@react-aria/i18n:isRTL-isRTL {
- localeString: string
- returnVal: undefined
-} |
rowProps: DOMAttributes, | ||
|
||
/** Props for the heading element, if any. */ | ||
headingProps: DOMAttributes, |
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.
Can we make this rowHeaderProps
to match the role?
@@ -245,7 +246,8 @@ function GridListInner<T extends object>({props, collection, gridListRef: ref}: | |||
values={[ | |||
[ListStateContext, state], | |||
[DragAndDropContext, {dragAndDropHooks, dragState, dropState}], | |||
[DropIndicatorContext, {render: GridListDropIndicatorWrapper}] | |||
[DropIndicatorContext, {render: GridListDropIndicatorWrapper}], | |||
[SectionContext, {name: 'GridListSection', render: GridListSectionInner}] |
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.
We can remove this SectionContext. This is only for backward compatibility in ListBox/Menu from when we used to have only a single <Section>
, but we don't need that here. You'll render a <GridListSection>
directly, not a <Section>
.
|
||
export interface GridListSectionProps<T> extends SectionProps<T> {} | ||
|
||
function GridListSectionInner<T extends object>(props: GridListSectionProps<T>, ref: ForwardedRef<HTMLElement>, section: Node<T>, className = 'react-aria-GridListSection') { |
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.
Based on above comment, you can merge this into GridListSection
|
||
return ( | ||
<div {...rowProps} > | ||
<header className="react-aria-Header" {...props} ref={ref}> |
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.
Having two elements will make this harder to style. Maybe we can use display: 'contents'
on the inner one so we have the correct ARIA structure, but you only need to style the outer one:
<header {...rowProps} ref={ref}>
<div {...rowHeaderProps} style={{display: 'contents'}}>
{children}
</div>
</header>
This would match the structure of GridListItem
too.
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.
linking related discussion #8667 (comment)
I think I like the display contents better
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.
+1, I did the same for the load more elements too (i.e. TableLoadMoreItem)
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.
still need to test the screenreader behavior but just some things I noted when scanning the code
*/ | ||
export function useGridListSection(props: AriaGridListSectionProps): GridListSectionAria { | ||
let {heading, 'aria-label': ariaLabel} = props; | ||
let headingId = useId(); |
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.
nit, but I wonder if we can omit passing the heading to the hook and instead leverage useSlotId
to generate this headingId
? That way either the user spreads headingProps
onto an element and thus the id
+ aria-labelledby
stick around or they don't and the id
doesn't get used
|
||
const GridListHeaderContext = createContext<HTMLAttributes<HTMLElement> | null>(null); | ||
|
||
export const GridListHeader = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { |
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.
I think this is ok, but the row
itself should get aria-attributes like aria-rowindex
when virtualized if so.
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
test using the two added stories in RAC. one is virtualized + dynamic, the other is non-virtualized static
🧢 Your Project: