-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Disabling TableView/ListView keyboard navigation and press interactions if collection is empty #3422
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
Disabling TableView/ListView keyboard navigation and press interactions if collection is empty #3422
Changes from all commits
07bdb89
e1d27e4
754ed24
790e797
378542d
1ddb884
4de7b36
6e9c29c
ca73890
1abf609
ad9cc9b
3e116d6
7c60e9d
9c0f617
ee651b0
d7a77c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,7 +93,8 @@ export function useGridCell<T, C extends GridCollection<T>>(props: GridCellProps | |
| isVirtualized, | ||
| focus, | ||
| shouldSelectOnPressUp, | ||
| onAction: onCellAction ? () => onCellAction(node.key) : onAction | ||
| onAction: onCellAction ? () => onCellAction(node.key) : onAction, | ||
| isDisabled: state.collection.size === 0 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it both non-interactive and non-navigable? or should we name the prop something else? or is that what you're suggesting in the comment?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR also covers ListView. Also, if the collection is empty, wouldn't there be no grid cells? I guess only in the case of tableview because of the headers?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, only TableView has grid cells when the collection is empty, CardView and TagGroup don't. Was just a bit worried that there might people using these grid hooks directly to build a table (or some other grid pattern) that wouldn't want to disable interactions when their table is empty |
||
| }); | ||
|
|
||
| let onKeyDownCapture = (e: ReactKeyboardEvent) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,11 +30,12 @@ export interface TableColumnResizeAria { | |
| export interface AriaTableColumnResizeProps<T> { | ||
| column: GridNode<T>, | ||
| label: string, | ||
| triggerRef: RefObject<HTMLDivElement> | ||
| triggerRef: RefObject<HTMLDivElement>, | ||
| isDisabled?: boolean | ||
| } | ||
|
|
||
| export function useTableColumnResize<T>(props: AriaTableColumnResizeProps<T>, state: TableState<T>, columnState: TableColumnResizeState<T>, ref: RefObject<HTMLInputElement>): TableColumnResizeAria { | ||
| let {column: item, triggerRef} = props; | ||
| let {column: item, triggerRef, isDisabled} = props; | ||
| const stateRef = useRef<TableColumnResizeState<T>>(null); | ||
| // keep track of what the cursor on the body is so it can be restored back to that when done resizing | ||
| const cursor = useRef<string | null>(null); | ||
|
|
@@ -159,7 +160,8 @@ export function useTableColumnResize<T>(props: AriaTableColumnResizeProps<T>, st | |
| stateRef.current.onColumnResizeEnd(item); | ||
| state.setKeyboardNavigationDisabled(false); | ||
| }, | ||
| onChange | ||
| onChange, | ||
| disabled: isDisabled | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than disabling this, shouldn't we just not render the menu button and not display the resizer on hover?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That should work, I just felt like it would be appropriate to give the user the ability to disable a column resizer but we can add that when a use case arises.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, since I ran into issues here I'm going to go back to the isDisabled approach |
||
| }, | ||
| ariaProps | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,7 +81,8 @@ interface TableContextValue<T> { | |
| state: TableState<T>, | ||
| layout: TableLayout<T>, | ||
| columnState: TableColumnResizeState<T>, | ||
| headerRowHovered: boolean | ||
| headerRowHovered: boolean, | ||
| isEmpty: boolean | ||
| } | ||
|
|
||
| const TableContext = React.createContext<TableContextValue<unknown>>(null); | ||
|
|
@@ -252,6 +253,7 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H | |
| return <TableSelectAllCell column={item} />; | ||
| } | ||
|
|
||
| // TODO: consider this case, what if we have hidden headers and a empty table | ||
| if (item.props.hideHeader) { | ||
| return ( | ||
| <TooltipTrigger placement="top" trigger="focus"> | ||
|
|
@@ -304,11 +306,13 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H | |
| setHorizontalScollbarVisible(bodyRef.current.clientHeight + 2 < bodyRef.current.offsetHeight); | ||
| } | ||
| }, []); | ||
| let {isFocusVisible, focusProps} = useFocusRing(); | ||
| let isEmpty = state.collection.size === 0; | ||
|
|
||
| return ( | ||
| <TableContext.Provider value={{state, layout, columnState, headerRowHovered}}> | ||
| <TableContext.Provider value={{state, layout, columnState, headerRowHovered, isEmpty}}> | ||
| <TableVirtualizer | ||
| {...gridProps} | ||
| {...mergeProps(gridProps, focusProps)} | ||
| {...styleProps} | ||
| className={ | ||
| classNames( | ||
|
|
@@ -337,13 +341,14 @@ function TableView<T extends object>(props: SpectrumTableProps<T>, ref: DOMRef<H | |
| onVisibleRectChange={onVisibleRectChange} | ||
| domRef={domRef} | ||
| bodyRef={bodyRef} | ||
| isFocusVisible={isFocusVisible} | ||
| getColumnWidth={columnState.getColumnWidth} /> | ||
| </TableContext.Provider> | ||
| ); | ||
| } | ||
|
|
||
| // This is a custom Virtualizer that also has a header that syncs its scroll position with the body. | ||
| function TableVirtualizer({layout, collection, focusedKey, renderView, renderWrapper, domRef, bodyRef, setTableWidth, getColumnWidth, onVisibleRectChange: onVisibleRectChangeProp, ...otherProps}) { | ||
| function TableVirtualizer({layout, collection, focusedKey, renderView, renderWrapper, domRef, bodyRef, setTableWidth, getColumnWidth, onVisibleRectChange: onVisibleRectChangeProp, isFocusVisible, ...otherProps}) { | ||
| let {direction} = useLocale(); | ||
| let headerRef = useRef<HTMLDivElement>(); | ||
| let loadingState = collection.body.props.loadingState; | ||
|
|
@@ -415,7 +420,8 @@ function TableVirtualizer({layout, collection, focusedKey, renderView, renderWra | |
|
|
||
| return ( | ||
| <div | ||
| {...mergeProps(otherProps, virtualizerProps)} | ||
| // Override virtualizer provided tabindex if TableView is empty so it is tabbable. | ||
| {...mergeProps(otherProps, virtualizerProps, collection.size === 0 && {tabIndex: 0})} | ||
|
Comment on lines
+423
to
+424
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. useVirtualizer will return tabindex=-1 if the focused view is still in the TableView, which is problematic for the empty case because the last focused view could be a column header prior to the TableView becoming empty and thus would still be in view. Debated putting this logic into useVirtualizer but I figured that would be too wide of a change. Specifically, in case someone was using Virtualizer to create a component that would keep some focusable views even when the collection was empty but they wanted those view to still be tabbable/didn't want the virtualizer to be tabbable |
||
| ref={domRef}> | ||
| <div | ||
| role="presentation" | ||
|
|
@@ -433,7 +439,15 @@ function TableVirtualizer({layout, collection, focusedKey, renderView, renderWra | |
| </div> | ||
| <ScrollView | ||
| role="presentation" | ||
| className={classNames(styles, 'spectrum-Table-body')} | ||
| className={ | ||
| classNames( | ||
| styles, | ||
| 'spectrum-Table-body', | ||
| { | ||
| 'focus-ring': isFocusVisible | ||
| } | ||
| ) | ||
| } | ||
| style={{flex: 1}} | ||
| innerStyle={{overflow: 'visible', transition: state.isAnimating ? `none ${state.virtualizer.transitionDuration}ms` : undefined}} | ||
| ref={bodyRef} | ||
|
|
@@ -461,17 +475,18 @@ function TableHeader({children, ...otherProps}) { | |
| function TableColumnHeader(props) { | ||
| let {column} = props; | ||
| let ref = useRef<HTMLDivElement>(null); | ||
| let {state} = useTableContext(); | ||
| let {state, isEmpty} = useTableContext(); | ||
| let {pressProps, isPressed} = usePress({isDisabled: isEmpty}); | ||
| let {columnHeaderProps} = useTableColumnHeader({ | ||
| node: column, | ||
| isVirtualized: true | ||
| }, state, ref); | ||
|
|
||
| let columnProps = column.props as SpectrumColumnProps<unknown>; | ||
|
|
||
| let {hoverProps, isHovered} = useHover(props); | ||
| let {hoverProps, isHovered} = useHover({...props, isDisabled: isEmpty}); | ||
|
|
||
| const allProps = [columnHeaderProps, hoverProps]; | ||
| const allProps = [columnHeaderProps, hoverProps, pressProps]; | ||
|
|
||
| return ( | ||
| <FocusRing focusRingClass={classNames(styles, 'focus-ring')}> | ||
|
|
@@ -483,6 +498,7 @@ function TableColumnHeader(props) { | |
| styles, | ||
| 'spectrum-Table-headCell', | ||
| { | ||
| 'is-active': isPressed, | ||
| 'is-resizable': columnProps.allowsResizing, | ||
| 'is-sortable': columnProps.allowsSorting, | ||
| 'is-sorted-desc': state.sortDescriptor?.column === column.key && state.sortDescriptor?.direction === 'descending', | ||
|
|
@@ -513,8 +529,9 @@ function TableColumnHeader(props) { | |
| } | ||
|
|
||
| let _TableColumnHeaderButton = (props, ref: FocusableRef<HTMLDivElement>) => { | ||
| let {isEmpty} = useTableContext(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the table is empty, maybe we just render a TableColumnHeader instead of one with a menu?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like we need to render the TableResizeColumnHeader if it is resizable, otherwise flipping from empty to non-empty doesn't replace the in view columns with resizable columns (caching probably?) |
||
| let domRef = useFocusableRef(ref); | ||
| let {buttonProps} = useButton({...props, elementType: 'div'}, domRef); | ||
| let {buttonProps} = useButton({...props, elementType: 'div', isDisabled: isEmpty}, domRef); | ||
| return ( | ||
| <div className={classNames(styles, 'spectrum-Table-headCellContents')}> | ||
| <FocusRing focusRingClass={classNames(styles, 'focus-ring')}> | ||
|
|
@@ -530,16 +547,17 @@ function ResizableTableColumnHeader(props) { | |
| let ref = useRef(null); | ||
| let triggerRef = useRef(null); | ||
| let resizingRef = useRef(null); | ||
| let {state, columnState, headerRowHovered} = useTableContext(); | ||
| let {state, columnState, headerRowHovered, isEmpty} = useTableContext(); | ||
| let stringFormatter = useLocalizedStringFormatter(intlMessages); | ||
| let {pressProps, isPressed} = usePress({isDisabled: isEmpty}); | ||
| let {columnHeaderProps} = useTableColumnHeader({ | ||
| node: column, | ||
| isVirtualized: true, | ||
| hasMenu: true | ||
| }, state, ref); | ||
| let {hoverProps, isHovered} = useHover(props); | ||
| let {hoverProps, isHovered} = useHover({...props, isDisabled: isEmpty}); | ||
|
|
||
| const allProps = [columnHeaderProps, hoverProps]; | ||
| const allProps = [columnHeaderProps, hoverProps, pressProps]; | ||
|
|
||
| let columnProps = column.props as SpectrumColumnProps<unknown>; | ||
|
|
||
|
|
@@ -589,7 +607,7 @@ function ResizableTableColumnHeader(props) { | |
| } | ||
| }, [columnState.currentlyResizingColumn, column.key]); | ||
|
|
||
| let showResizer = headerRowHovered || columnState.currentlyResizingColumn != null; | ||
| let showResizer = !isEmpty && (headerRowHovered || columnState.currentlyResizingColumn != null); | ||
|
|
||
| return ( | ||
| <FocusRing focusRingClass={classNames(styles, 'focus-ring')}> | ||
|
|
@@ -601,6 +619,7 @@ function ResizableTableColumnHeader(props) { | |
| styles, | ||
| 'spectrum-Table-headCell', | ||
| { | ||
| 'is-active': isPressed, | ||
| 'is-resizable': columnProps.allowsResizing, | ||
| 'is-sortable': columnProps.allowsSorting, | ||
| 'is-sorted-desc': state.sortDescriptor?.column === column.key && state.sortDescriptor?.direction === 'descending', | ||
|
|
@@ -685,7 +704,6 @@ function TableSelectAllCell({column}) { | |
| } | ||
| <Checkbox | ||
| {...checkboxProps} | ||
| isDisabled={isSingleSelectionMode} | ||
| isEmphasized | ||
| UNSAFE_style={isSingleSelectionMode ? {visibility: 'hidden'} : undefined} | ||
| UNSAFE_className={classNames(styles, 'spectrum-Table-checkbox')} /> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.