feat: add highlight selection to S2 TableView#9883
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Approving for testing on Monday, some comments that can be follow up if need be
| function isPrevSelected(id: Key | undefined, state: TreeState<unknown>) { | ||
| if (id == null || !state) { | ||
| return false; | ||
| } | ||
| let keyBefore = state.collection.getKeyBefore(id); | ||
| return keyBefore != null && state.selectionManager.isSelected(keyBefore); | ||
| } |
There was a problem hiding this comment.
just to make sure, walking backwards doesn't have the same problem as walking forwards? Just noticing the difference in logic vs isNextSelected
There was a problem hiding this comment.
yeah pretty much. for getKeyBefore, i think you could really only get a content node returned if an expanded item's lastChildKey was a content node but that shouldn't happen
| // Sometimes the last key is a loader node, so we check the previous nodes | ||
| while (node && node.type !== 'item') { | ||
| let prevKey = node.prevKey; | ||
| node = prevKey ? state.collection.getItem(prevKey) : null; | ||
| } |
There was a problem hiding this comment.
I noticed this changed from the previous implementation, seems to have affected the border for the last row in a quiet ListView when loading
I think this is the only style this affects, but I think the previous behavior was correct? Any other reason for this update in implementation?
There was a problem hiding this comment.
hm it was mainly done out of precaution but it wasn't broken before per say. if the before is what we wanted, we can remove this
|
Build successful! 🎉 |
| isSelected: '--borderColorBlue' | ||
| } | ||
| }, | ||
| // When highlight selection, render gray dividers as box shadow |
There was a problem hiding this comment.
why only for highlight selection?
There was a problem hiding this comment.
hmm i wanted to leave checkbox selection alone as much as possible and leaned towards the idea that if we could use borders we should use them. the box shadows feel more like a workaround
There was a problem hiding this comment.
i mean why do we need box hadows for highlight selection?
| forcedColorAdjust: 'none' | ||
| }); | ||
|
|
||
| const highlightSelectionBorder = css( |
There was a problem hiding this comment.
do we have to use a before for this or could we add an extra element?
| return keyAfter != null && state.selectionManager.isSelected(keyAfter); | ||
| } | ||
|
|
||
| function isPrevSelected(id: Key | undefined, state: TreeState<unknown>) { |
There was a problem hiding this comment.
Do we want to share these utils between collections? I see you exported them from TableView
There was a problem hiding this comment.
yeah i originally had them in a utils file but there were concerns that the collections getKeyAfter/Before/etc might not be consistent across each other. for example, TreeView had its own isNextSelected bc of the issue of running into content nodes which isn't the case for ListView or TableView
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
yihuiliao
dismissed stale reviews from snowystinger, devongovett, and LFDanLu
via
ca13c53
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:RowRenderProps RowRenderProps {
allowsDragging?: boolean
hasChildItems: boolean
id?: Key
isDisabled: boolean
isDragging?: boolean
isDropTarget?: boolean
isExpanded: boolean
isFocusVisible: boolean
isFocusVisibleWithin: boolean
isFocused: boolean
isHovered: boolean
isPressed: boolean
isSelected: boolean
level: number
selectionBehavior: SelectionBehavior
selectionMode: SelectionMode
+ state: TableState<unknown>
}@react-spectrum/s2/@react-spectrum/s2:TableView TableView {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
children?: ReactNode
defaultExpandedKeys?: Iterable<Key>
defaultSelectedKeys?: 'all' | Iterable<Key>
density?: 'compact' | 'spacious' | 'regular' = 'regular'
disabledBehavior?: DisabledBehavior = 'all'
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
dragAndDropHooks?: DragAndDropHooks
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
expandedKeys?: Iterable<Key>
id?: string
isQuiet?: boolean
loadingState?: LoadingState
onAction?: (Key) => void
onExpandedChange?: (Set<Key>) => any
onLoadMore?: () => any
onResize?: (Map<Key, ColumnSize>) => void
onResizeEnd?: (Map<Key, ColumnSize>) => void
onResizeStart?: (Map<Key, ColumnSize>) => void
onSelectionChange?: (Selection) => void
onSortChange?: (SortDescriptor) => any
overflowMode?: 'wrap' | 'truncate' = 'truncate'
renderActionBar?: ('all' | Set<Key>) => ReactElement
selectedKeys?: 'all' | Iterable<Key>
selectionMode?: SelectionMode
+ selectionStyle?: 'checkbox' | 'highlight' = 'checkbox'
shouldSelectOnPressUp?: boolean
slot?: string | null
sortDescriptor?: SortDescriptor
styles?: StylesPropWithHeight
}/@react-spectrum/s2:TableViewProps TableViewProps {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
children?: ReactNode
defaultExpandedKeys?: Iterable<Key>
defaultSelectedKeys?: 'all' | Iterable<Key>
density?: 'compact' | 'spacious' | 'regular' = 'regular'
disabledBehavior?: DisabledBehavior = 'all'
disabledKeys?: Iterable<Key>
disallowEmptySelection?: boolean
dragAndDropHooks?: DragAndDropHooks
escapeKeyBehavior?: 'clearSelection' | 'none' = 'clearSelection'
expandedKeys?: Iterable<Key>
id?: string
isQuiet?: boolean
loadingState?: LoadingState
onAction?: (Key) => void
onExpandedChange?: (Set<Key>) => any
onLoadMore?: () => any
onResize?: (Map<Key, ColumnSize>) => void
onResizeEnd?: (Map<Key, ColumnSize>) => void
onResizeStart?: (Map<Key, ColumnSize>) => void
onSelectionChange?: (Selection) => void
onSortChange?: (SortDescriptor) => any
overflowMode?: 'wrap' | 'truncate' = 'truncate'
renderActionBar?: ('all' | Set<Key>) => ReactElement
selectedKeys?: 'all' | Iterable<Key>
selectionMode?: SelectionMode
+ selectionStyle?: 'checkbox' | 'highlight' = 'checkbox'
shouldSelectOnPressUp?: boolean
slot?: string | null
sortDescriptor?: SortDescriptor
styles?: StylesPropWithHeight
} |
Closes
you can reference the figma but it might be out of date. notable differences are:
chromatic: https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=1191
✅ Pull Request Checklist:
📝 Test Instructions:
test highlight selection in s2 storybook + docs. we discussed with design to align the highlight selection styles with TreeView/ListView. the borders should have rounded borders (matching TreeView/ListView) and now have a focus ring rather than a focus indicator. there should not be gray dividers between rows when there is a selected group. spot check checkbox selection to make sure it hasn't changed. make sure to test dnd + highlight selection AND dnd + checkbox selection
🧢 Your Project: