Adding automatic scrolling on item focus for non-virtualized collections#2232
Conversation
allows non virtualized tables/grids created with useTable/useGrid to have automatic scrolling when moving focus via arrow keys
some components will have a ref that is not the same element as the scrollable region so add a prop for scrollRef
LFDanLu
requested review from
dannify,
devongovett and
snowystinger
as code owners
| listStyle: "none", | ||
| maxHeight: "150px", | ||
| overflow: "auto" |
There was a problem hiding this comment.
the async example in useComboBox docs will now scroll, also is a good thing to add to the example since people will most likely want to limit the listbox height
There was a problem hiding this comment.
Can you add this to the useSelect example as well? The listbox there should be identical I believe.
| import {useFilter} from '@react-aria/i18n'; | ||
| import {useListBox, useOption} from '@react-aria/listbox'; | ||
|
|
||
| export function ComboBox(props) { |
There was a problem hiding this comment.
You can essentially ignore this code, copy pasta'd from our aria hook example. If we feel the docs example is sufficient for testing I can remove this, but feels useful for when we wanna test some more hook specific behavior in the future (e.g. non virtualized combobox)
There was a problem hiding this comment.
I like this. I think we should probably have more stories for the docs example. Also helps us ensure TS actually passes 😉
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
|
|
||
| // If not virtualized, scroll the focused element into view when the focusedKey changes. |
There was a problem hiding this comment.
The code below is from useSelectableList
|
Build successful! 🎉 |
| </TableHeaderRow> | ||
| ))} | ||
| </TableRowGroup> | ||
| <TableRowGroup ref={bodyRef} type="tbody" style={{display: 'block', overflow: 'auto', maxHeight: '200px'}}> |
There was a problem hiding this comment.
The same code from the useTable aria example except for the changes to make the table body scrollable and changes to work with the useSelectableCollection changes
The table column positioning is messed up due to the display: block hence why this is in a story and not the docs.
| isVirtualized, | ||
| scrollRef: ref |
There was a problem hiding this comment.
Opinions on this? I added this here because end users could make their own TabList scrollable instead of following our collapse behavior
There was a problem hiding this comment.
Yeah seems reasonable. I'm having a hard time imagining a virtualized tab list though. Maybe leave off that option for now?
| let {tabListProps} = useTabList(props, state, ref); | ||
| return ( | ||
| <div style={{height: '150px'}}> | ||
| <div {...tabListProps} ref={ref} style={{display: 'flex', borderBottom: '1px solid grey', maxWidth: '400px', overflow: 'auto'}}> |
There was a problem hiding this comment.
Taken from TabList aria example
| isVirtualized, | ||
| scrollRef: ref |
There was a problem hiding this comment.
Yeah seems reasonable. I'm having a hard time imagining a virtualized tab list though. Maybe leave off that option for now?
| allowsTabNavigation = false | ||
| allowsTabNavigation = false, | ||
| isVirtualized, | ||
| scrollRef |
There was a problem hiding this comment.
Can we default this to ref since they will be the same in many cases?
| getRowText?: (key: Key) => string | ||
| getRowText?: (key: Key) => string, | ||
| /** | ||
| * The ref attached to the scrollable body. Used to provided automatic scrolling on item focus for non-virtualized grids. |
There was a problem hiding this comment.
| * The ref attached to the scrollable body. Used to provided automatic scrolling on item focus for non-virtualized grids. | |
| * The ref attached to the scrollable body. Used to provide automatic scrolling on item focus for non-virtualized grids. |
| /** | ||
| * Whether the TabList is contained in a virtual scroller. | ||
| */ | ||
| isVirtualized?: boolean |
There was a problem hiding this comment.
Can we remove this option for now?
There was a problem hiding this comment.
AHH, my bad forgot to remove it from the types
|
Build successful! 🎉 |
| let lotsOfItems: any[] = []; | ||
| for (let i = 0; i < 50; i++) { | ||
| lotsOfItems.push({name: 'Item ' + i}); | ||
| } |
There was a problem hiding this comment.
you don't need to do anything with this, but a fast way to do this is
let lotsOfItems = (new Array(50)).map((_, i) => `Item ${i}` )
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
lgtm everything scrolling like i would think it should
|
Build successful! 🎉 |
4 participants
Closes #2156
Essentially shifts some useSelectableList logic to useSelectableCollection.
Note that this doesn't handle the shift tabbing issue yet, that will be in a separate PR for discussion (#2233)
✅ Pull Request Checklist:
📝 Test Instructions:
There are a couple new stories (useComboBox, useTabList, and useTable) that have non-virtualized collections. Test and make sure that keyboard navigation scrolls items into view as you focus them
Some interactions to try:
🧢 Your Project:
RSP