Support links in collection components #4993
Conversation
# Conflicts: # packages/@react-aria/interactions/src/usePress.ts
# Conflicts: # packages/react-aria-components/src/TagGroup.tsx # packages/react-aria-components/stories/index.stories.tsx
# Conflicts: # packages/@react-spectrum/menu/src/Menu.tsx # packages/@react-spectrum/table/stories/Table.stories.tsx # packages/react-aria-components/src/Collection.tsx
This comment was marked as outdated.
This comment was marked as outdated.
|
Build successful! 🎉 |
|
|
||
| return ( | ||
| <RouterProvider navigate={setUrl}> | ||
| <Tabs selectedKey={url}> |
There was a problem hiding this comment.
just clarifying
this is controlled, but we don't listen for onSelectionChange, instead, we listen to navigate on a parent component? if someone hooks up an onSelectionChange, is that a big deal? What happens if some aren't links?
There was a problem hiding this comment.
I'm probably just mixing things up, but https://reactspectrum.blob.core.windows.net/reactspectrum/4e6aef8500d9a5a378432495ce9b2de09883112d/storybook/index.html?path=/story/listview-selection--links&args=selectionMode:single&providerSwitcher-express=false
I should be able to do selection and navigation when selectionMode is single and selectionStyle is checkbox?
What I'm seeing is onSelectionChange in action panel, but no visual confirmation of selection, and I can't do the navigation.
In selectionmode single and selectionstyle highlight, if I double click on an item in that same story, then I can get stuck in selection mode. I think this is the combination that is supposed to be broken though?
Can we add a story for Tags with links and onRemove?
I'm happy for all of this to be followup, so approving, I'd like it to be in testing session.
# Conflicts: # packages/@react-spectrum/list/test/ListView.test.js
|
Build successful! 🎉 |
| if (linkBehavior === 'selection') { | ||
| router.open(ref.current, e); | ||
| // Always set selected keys back to what they were so that select and combobox close. | ||
| manager.setSelectedKeys(manager.selectedKeys); |
There was a problem hiding this comment.
Not totally sure about this because it'll also result in onSelectionChange being fired...
There was a problem hiding this comment.
I think this is ok since it is just setting it back to what it was. If we are worried about it firing onSelectionChange even though a link action was fired instead, I don't think it is that bad to keep the dropdown open for select/combobox since the page will be rerouted with a default click. If the user uses command + click then keeping the menu open is similar to the experience if they performed a right click -> open in new tab interaction
|
Build successful! 🎉 |
There was a problem hiding this comment.
Still looking through the PR, here are my thoughts on the open questions.
In what package should we put RouterProvider?
Perhaps we should put it in the link package to follow the same precedent we have for I18nProvider (aka it is in @react-aria/i18n)? That being said, the RouterProvider isn't related to the useLink hook at all so I guess it is fine to keep it in @react-aria/utils. Maybe a new package just for general Providers/contexts (not sure if we have/will have any other good candidate to live in a new package though)
Should we integrate RouterProvider into the Spectrum Provider component?
I think we should, otherwise people could forget to add it to their apps and then things would break when they upgrade right?
There was a problem hiding this comment.
LGTM, did a brief sweep of the behavior and the logic looks good to me. I see we sometimes are calling the preexisitng event callbacks (onAction, onSelectionChange, etc) upon link navigation (aka highlight selection tables/combobox) but other times we don't (listbox), should we be worried about the inconsistency? Alternatively should we provide a standardized callback when link navigation is triggered via press? Not sure how hard it would be to make things consistent/provide this callback though
Provisional approval pending feedback on the open questions in the PR
| if (linkBehavior === 'selection') { | ||
| router.open(ref.current, e); | ||
| // Always set selected keys back to what they were so that select and combobox close. | ||
| manager.setSelectedKeys(manager.selectedKeys); |
There was a problem hiding this comment.
I think this is ok since it is just setting it back to what it was. If we are worried about it firing onSelectionChange even though a link action was fired instead, I don't think it is that bad to keep the dropdown open for select/combobox since the page will be rerouted with a default click. If the user uses command + click then keeping the menu open is similar to the experience if they performed a right click -> open in new tab interaction
I think
That'd increase the number of props people send to our Provider, which is currently about visuals/langauge/etc, it's not about navigation at the moment. I'd prefer a separate provider which is specific to this. It'd also make it easier for people to keep our Provider at the top, but they could handle routing lower down. |
I think because links override selection in listbox (i.e. link items are not selectable)? |
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } @react-aria/gridlistAriaGridListOptions AriaGridListOptions<T> {
disabledBehavior?: DisabledBehavior
isVirtualized?: boolean
keyboardDelegate?: KeyboardDelegate
+ linkBehavior?: 'action' | 'selection' | 'override' = 'action'
onAction?: (Key) => void
shouldFocusWrap?: boolean = false
}it changed:
@react-aria/listboxAriaListBoxProps AriaListBoxOptions<T> {
isVirtualized?: boolean
keyboardDelegate?: KeyboardDelegate
+ linkBehavior?: 'action' | 'selection' | 'override' = 'override'
shouldFocusOnHover?: boolean
shouldSelectOnPressUp?: boolean
shouldUseVirtualFocus?: boolean
}@react-aria/selectionAriaSelectableCollectionOptions AriaSelectableCollectionOptions {
allowsTabNavigation?: boolean
autoFocus?: boolean | FocusStrategy = false
disallowEmptySelection?: boolean = false
disallowSelectAll?: boolean = false
disallowTypeAhead?: boolean = false
isVirtualized?: boolean
keyboardDelegate: KeyboardDelegate
+ linkBehavior?: 'action' | 'selection' | 'override' = 'action'
ref: RefObject<HTMLElement>
scrollRef?: RefObject<HTMLElement>
selectOnFocus?: boolean = false
selectionManager: MultipleSelectionManager
shouldUseVirtualFocus?: boolean
}
it changed:
AriaSelectableListOptions AriaSelectableListOptions {
- allowsTabNavigation?: boolean
- autoFocus?: boolean | FocusStrategy = false
collection: Collection<Node<unknown>>
disabledKeys: Set<Key>
- disallowEmptySelection?: boolean = false
- disallowTypeAhead?: boolean = false
- isVirtualized?: boolean
keyboardDelegate?: KeyboardDelegate
- ref?: RefObject<HTMLElement>
- selectOnFocus?: boolean = false
- selectionManager: MultipleSelectionManager
- shouldFocusWrap?: boolean = false
- shouldUseVirtualFocus?: boolean
}it changed:
SelectableItemOptions SelectableItemOptions {
allowsDifferentPressOrigin?: boolean
focus?: () => void
isDisabled?: boolean
isVirtualized?: boolean
key: Key
+ linkBehavior?: 'action' | 'selection' | 'override' | 'none' = 'action'
onAction?: () => void
ref: RefObject<FocusableElement>
selectionManager: MultipleSelectionManager
shouldSelectOnPressUp?: boolean
}
it changed:
@react-aria/utilsfilterDOMProps filterDOMProps {
- props: (DOMProps & AriaLabelingProps)
+ props: (DOMProps & AriaLabelingProps & LinkDOMProps)
opts: Options
returnVal: undefined
}useFormReset-
+openLink {
+ target: HTMLAnchorElement
+ modifiers: Modifiers
+ setOpening: any
+ returnVal: undefined
+}openLink-
+getSyntheticLinkProps {
+ props: LinkDOMProps
+ returnVal: undefined
+}getSyntheticLinkProps-
+RouterProvider {
+ children: ReactNode
+ navigate: (string) => void
+}RouterProvider-
+useRouter {
+ returnVal: undefined
+}@react-stately/selectionMultipleSelectionManager MultipleSelectionManager {
canSelectItem: (Key) => boolean
childFocusStrategy: FocusStrategy
clearSelection: () => void
disabledBehavior: DisabledBehavior
disabledKeys: Set<Key>
disallowEmptySelection?: boolean
extendSelection: (Key) => void
firstSelectedKey: Key | null
focusedKey: Key
isDisabled: (Key) => boolean
isEmpty: boolean
isFocused: boolean
+ isLink: (Key) => boolean
isSelectAll: boolean
isSelected: (Key) => boolean
isSelectionEqual: (Set<Key>) => boolean
lastSelectedKey: Key | null
select: (Key, PressEvent | LongPressEvent | PointerEvent) => void
selectAll: () => void
selectedKeys: Set<Key>
selectionBehavior: SelectionBehavior
selectionMode: SelectionMode
setFocused: (boolean) => void
setFocusedKey: (Key, FocusStrategy) => void
setSelectedKeys: (Iterable<Key>) => void
setSelectionBehavior: (SelectionBehavior) => void
toggleSelectAll: () => void
toggleSelection: (Key) => void
}
SelectionManager SelectionManager {
canSelectItem: (Key) => void
childFocusStrategy: FocusStrategy
clearSelection: () => void
constructor: (Collection<Node<unknown>>, MultipleSelectionState, SelectionManagerOptions) => void
disabledBehavior: DisabledBehavior
disabledKeys: Set<Key>
disallowEmptySelection: boolean
extendSelection: (Key) => void
firstSelectedKey: Key | null
focusedKey: Key
isDisabled: (Key) => void
isEmpty: boolean
isFocused: boolean
+ isLink: (Key) => void
isSelectAll: boolean
isSelected: (Key) => void
isSelectionEqual: (Set<Key>) => void
lastSelectedKey: Key | null
replaceSelection: (Key) => void
select: (Key, PressEvent | LongPressEvent | PointerEvent) => void
selectAll: () => void
selectedKeys: Set<Key>
selectionBehavior: SelectionBehavior
selectionMode: SelectionMode
setFocused: (boolean) => void
setFocusedKey: (Key | null, FocusStrategy) => void
setSelectedKeys: (Iterable<Key>) => void
setSelectionBehavior: (SelectionBehavior) => void
toggleSelectAll: () => void
toggleSelection: (Key) => void
}
|
Closes #1244, fixes #2595
This adds support for links in all collection components. The
<Item>component supportshrefand other link-related DOM props. When anhrefis provided, the item becomes a link and is rendered as an<a>element when possible. There are three behaviors for links in collections:onActionprop. In the default checkbox selection mode, that means clicking an item navigates by default, and if you click the checkbox you start selecting. In highlight selection mode, clicking once selects and double clicking navigates. Links are basically implemented as a default action.Components using the aria grid pattern do not use native
<a>elements becauserole="row"is not allowed on an<a>element according to the HTML spec. In this case, we put the link attributes on the regular row element as data attributes, and trigger the link using JavaScript on click. You lose the ability to right click and perform link actions, but until the spec is changed, this is the best we can do. w3c/html-aria#473To integrate with client side routers, there is a
RouterProvidercomponent. This accepts anavigateprop that you provide a function from your router (e.g. react-router, next.js, etc.) to go to a new url. All components that support being links use this provider to do their navigation. If one isn't provided, native browser navigation is performed by simulating a click on the link element. That is also done when modifier keys are pressed. This approach means you only need to provide the router provider in one place at the top of your app, and we don't need to open up the API to pass in custom Link components which comes with a host of potential issues. In the future, RouterProvide may also allow us to implement other features such as automatically determining the selected item (e.g. tab) based on the current URL so you don't need to pass aselectedKeyat all.Open questions