Skip to content

Support grid layout and tab navigation in GridList #6486

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

Merged
merged 5 commits into from
Jun 8, 2024
Merged

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Jun 3, 2024

Related to #4742

This adds a layout prop to the RAC GridList component, which can be set to "stack" (default) or "grid". When set to "grid", arrow key navigation is used to move left and right between columns. To access focusable elements inside the items, tab can be used instead. The tab sequence goes through elements within the focused item before going out of the collection.

This can also be enabled even for stack layouts via the keyboardNavigationBehavior="tab" prop. For example, if you have a list where interactive elements within items aren't arranged horizontally, tab might make more sense.

* via the left/right arrow keys or the tab key.
* @default 'arrow'
*/
navigationBehavior?: 'arrow' | 'tab'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems pretty good to me, matches linkBehavior which indicates that the way you interact with an element changes due to this prop

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would lean towards keyboardNavigationBehavior, although it's longer, it avoids confusion with router navigation-related props.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah that's true, good point

...props,
keyboardDelegate,
// Only tab navigation is supported in grid layout.
navigationBehavior: layout === 'grid' ? 'tab' : navigationBehavior
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confusing? I originally only had layout as a prop, but realized that navigationBehavior="tab" might be useful for some cases even in stack layout so now it's two props. But I don't think arrow navigation in grid layout doesn't really make sense? But maybe? I guess table does that...

@rspbot
Copy link

rspbot commented Jun 3, 2024

@majornista
Copy link
Collaborator

majornista commented Jun 3, 2024

This is useful. Can you add a Storybook control for this so that it's easier to demo the behavior with stack layout?

Testing, locally doesn't seem like navigationBehavior="tab" works as you describe with layout="stack", the descendant button does not receive focus on Tab.

@rspbot
Copy link

rspbot commented Jun 4, 2024

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general works well and as expected (matches TableView for where focus is restored when coming back into the collection). I think it's a good solution to the problem.

gridTemplate: args.layout === 'grid' ? 'repeat(3, 1fr) / repeat(3, 1fr)' : 'auto / 1fr',
gridAutoFlow: 'row'
}}>
<MyGridListItem>1,1 <Button>Actions</Button></MyGridListItem>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do items know what is their index in the data source?
do items know what is their position in the visible viewport/row?

@jaknas
Copy link

jaknas commented Jun 4, 2024

Does the drag and drop logic need any updates or modifications?

@devongovett
Copy link
Member Author

@jaknas I don't think so. This line enables grid layout support, which is already available in ListBox: https://github.com/adobe/react-spectrum/pull/6486/files#diff-87753d6f055505c1f4a4cb6698abe34276d6aa0c17bee1f8b9ec509c4e22944aR169

@rspbot
Copy link

rspbot commented Jun 4, 2024

reidbarber
reidbarber previously approved these changes Jun 6, 2024
@rspbot
Copy link

rspbot commented Jun 7, 2024

majornista
majornista previously approved these changes Jun 7, 2024
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thing I noticed w/ regards to RTL, but otherwise this looks good to me

disabledKeys,
disabledBehavior,
layout,
direction: 'ltr'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be sourced from useLocale instead of being ltr all the time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, yeah! good catch

@devongovett devongovett dismissed stale reviews from majornista and reidbarber via 1ff482c June 8, 2024 00:11
@rspbot
Copy link

rspbot commented Jun 8, 2024

@rspbot
Copy link

rspbot commented Jun 8, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-aria/gridlist

AriaGridListOptions

 AriaGridListOptions<T> {
   disabledBehavior?: DisabledBehavior
   isVirtualized?: boolean
   keyboardDelegate?: KeyboardDelegate
+  keyboardNavigationBehavior?: 'arrow' | 'tab' = 'arrow'
   linkBehavior?: 'action' | 'selection' | 'override' = 'action'
   onAction?: (Key) => void
   shouldFocusWrap?: boolean = false
 }

it changed:

  • useGridList

AriaGridListProps

 AriaGridListProps<T> {
   disabledBehavior?: DisabledBehavior
+  keyboardNavigationBehavior?: 'arrow' | 'tab' = 'arrow'
   onAction?: (Key) => void
 }

@devongovett devongovett merged commit a857ec7 into main Jun 8, 2024
26 checks passed
@devongovett devongovett deleted the gridlist-layout branch June 8, 2024 01:10
...props,
keyboardDelegate,
// Only tab navigation is supported in grid layout.
keyboardNavigationBehavior: layout === 'grid' ? 'tab' : keyboardNavigationBehavior
Copy link

@tryggvigy tryggvigy Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if 'arrow' should be allowed in grid layouts so for example https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/layout-grids/#ex2_label can be implemented.

Having the ability to implement the following behaviour for arrow key navigations in grid layout lists regardless of orientation (vertical or horizontal), since layout is only relevant for sighted users, seems useful:

  • left/right: navigate to next focusable element, either within the item, or in the next/prev item.
  • up/down: Move to prev/next item and attempt to land on the corresponding element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants