ListView hook refactor#3086
Conversation
Felt that the previous modifications to GridKeyboardDelegate were too specific to ListView so split out the changes. Also fixes pageUp/Down/Home/End when you are focused on the ListView row (we forgot to pass the layout to the delegate previously)
needed to override getItemRect for the ListView keyboard delegate so that it always uses the row key, namely due to the gridcell keys being unavailable in the listlayout. Changed getItem and getItemRect to protected funcs in GridKeyboardDelegate so I could do that
still need to fix pageUp/down
needed to override getKeyLeftOf/getKeyRightOf so they dont use getFirstKey or getLastKey or else focus would jump to the last row instead of wrapping inside the cell
also reverts changes to other files that arent necessary anymore. Adds tests for disabled keys
…listview_row_focus_2
…listview_row_focus_2
helps avoid weird test cases like #3000 (comment)
|
Build successful! 🎉 |
solimant
left a comment
solimant
left a comment
There was a problem hiding this comment.
Looks good, just one minor comment 👍🏼
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| jest.mock('@react-aria/live-announcer'); |
There was a problem hiding this comment.
Jest hoists jest.mock calls to the top of the module and before any imports. So, it should be okay to push this line down after the imports block, and Jest will still call it first (unless that's the coding style we're following in our tests).
EDIT: looks like that's the coding style we're following 😅
LFDanLu
dismissed stale reviews from reidbarber, solimant, and snowystinger
via
0f73c40
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
|
||
| export interface ListViewAria { | ||
| /** Props for the grid element. */ | ||
| gridProps: HTMLAttributes<HTMLElement> |
There was a problem hiding this comment.
I think gridProps. We tend to match the role, and if we changed this one it wouldn't match with rowProps/cellProps from useListItem.
| let selection = state.selectionManager.rawSelection; | ||
| let lastSelection = useRef(selection); | ||
| useUpdateEffect(() => { |
There was a problem hiding this comment.
I think this should remain in the grid package (I think the live announcer wasn't necessary for other roles with selection, e.g. listbox?), and be exported as a separate hook.
| let id = useId(); | ||
| listMap.set(state, {id, onAction}); | ||
|
|
||
| // This is useHighlightSelectionDescription copy pasted, it isn't exposed by react-aria/grid. |
There was a problem hiding this comment.
I think I would just expose it from the grid package for now. The solution might be different for other roles. The reason the description is on the whole grid and not each individual row is due to limitations of VoiceOver, for listbox or other roles it works better.
| /** Whether selection should occur on press up instead of press down. */ | ||
| shouldSelectOnPressUp?: boolean, | ||
| /** Whether the list item is disabled. */ | ||
| isDisabled?: boolean |
There was a problem hiding this comment.
I think this shouldn't be necessary since we have disabledKeys. I believe this option was legacy in grid.
| focus | ||
| }); | ||
|
|
||
| let onKeyDown = (e: ReactKeyboardEvent) => { |
There was a problem hiding this comment.
Worth it to extract this keyboard navigation stuff into a separate hook?
There was a problem hiding this comment.
Yeah it would be useful for other single column grid like components, I can pull it out into its own hook when we need it. Not sure there is one that exists right now though (TagGroup handles left/right differently), but I maybe forgetting one
|
I think due to the duplicated translations, we should expose more things from the grid package to reuse. I am now thinking of the grid package as something we wouldn't document directly, more of a lower level building block for components like table and list. So I think it's ok to expose more pieces from there that we can reuse in components that use the grid role. |
|
@devongovett sounds good, mind if I handle those in a followup? I think it will make the code diff much cleaner since there won't be a bunch of ListView specific refactor code changes then. |
|
Yeah sure |
|
Build successful! 🎉 |
| // TODO: Figure out the typescript for this | ||
| // @ts-ignore | ||
| if (y >= r.y + 10 && y <= r.maxY - 10 && collection.getItem(closest.key).value.type === 'folder') { |
There was a problem hiding this comment.
ts-ignoring this for now because value.type doesn't fit the generic type that value gets here. Will update the logic when we decide whether or not to make the type more restrictive for ListView (aka user has to provide type to their items) or if we change this logic overall
There was a problem hiding this comment.
Turns out value.type is not alway going to be defined, but only specifically for static ListViews which I don't think makes sense w/ drag and drop in general. Perhaps just to be safer we should change this to be collection.getItem(closest.key).hasChildNodes but we should discuss what the true restrictions for "on" drops should be. Will handle in followup
found when testing screen reader announcements
|
Build successful! 🎉 |
|
Build successful! 🎉 |
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
everything appears to be in working order, re-approving
6 participants
Relies on #3000
✅ Pull Request Checklist:
📝 Test Instructions:
Quite the refactor so ListView need to be throughly retested, keyboard behavior especially.
🧢 Your Project:
RSP