Skip to content
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

Add support for routerOptions and useHref #5864

Merged
merged 7 commits into from
Apr 3, 2024
Merged

Add support for routerOptions and useHref #5864

merged 7 commits into from
Apr 3, 2024

Conversation

devongovett
Copy link
Member

@devongovett devongovett commented Feb 13, 2024

Closes #5395, closes #5335, relates to #5476

This improves integration with client side routers by introducing two new APIs:

  • A routerOptions prop on each component that supports links. This is an opaque object that React Aria simply passes through to the router. It can be used for options like controlling scroll behavior, using replaceState instead of pushState, or any other features offered by a router. It is passed to the navigate function provided to RouterProvider as a second argument in addition to the href.
  • A useHref prop for RouterProvider which is used to convert an href provided to a link component to a native href. For example, a router might accept hrefs relative to a base path, or offer additional custom ways of specifying link destinations. The original href specified on the link is passed to the navigate function of the RouterProvider, and useHref is used to generate the full native href to put on the actual DOM element.

Both of these APIs work with TypeScript to enable autocomplete and type checking for the router options and also the href. To configure it, you can use TypeScript module augmentation to specify the RouterOptions type:

declare module "react-aria-components" {
  interface RouterOptions {
    href: MyRouterHref,
    routerOptions: MyRouterOptions
  }
}

Do that once in your app (maybe where you setup RouterProvider), and all link components in React Aria/RSP will use those options.

To do

  • Docs

@rspbot
Copy link

rspbot commented Feb 13, 2024

# Conflicts:
#	packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js
#	packages/@react-spectrum/combobox/test/ComboBox.test.js
#	packages/@react-spectrum/link/test/Link.test.js
#	packages/@react-spectrum/list/test/ListView.test.js
#	packages/@react-spectrum/listbox/test/ListBox.test.js
#	packages/@react-spectrum/picker/test/Picker.test.js
@rspbot
Copy link

rspbot commented Feb 15, 2024

@@ -494,4 +494,8 @@ export class SelectionManager implements MultipleSelectionManager {
isLink(key: Key) {
return !!this.collection.getItem(key)?.props?.href;
}

getItemProps(key: Key) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this to selection manager feels a bit weird, but not sure where else to put it. useSelectableCollection and useSelectableItem don't currently receive the collection as props, except via SelectionManager...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, it's a bit odd, however, we already have 'isLink' in here, which is almost the same thing and is only tangentially related to selection

would it make more sense to just return the collection and then handle this specific logic outside?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, the original idea was that the MultipleSelectionManager interface could have multiple implementations, perhaps not even using collections under the hood. So if we expose the collection from the interface that will force other implementations to also use collections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense :-/

snowystinger
snowystinger previously approved these changes Feb 15, 2024
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can keep discussing, but approving so we can get nightlies out sooner rather than later

LFDanLu
LFDanLu previously approved these changes Feb 16, 2024
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs/code look good to me, will test the apps once they get built on main. Does the remix one get built somewhere? I tried running it locally but got a Invalid URL error. Otherwise just one small comment but happy to get this merged as is

Comment on lines +160 to +170
export function useLinkProps(props: LinkDOMProps) {
let router = useRouter();
return {
href: props?.href ? router.useHref(props?.href) : undefined,
target: props?.target,
rel: props?.rel,
download: props?.download,
ping: props?.ping,
referrerPolicy: props?.referrerPolicy
};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit but it almost feels like this useRouter call should be in filterDOMProps instead of being a separate function if only to guard against cases where both filterDOMProps and useLinkProps are called and the props from useLinkProps need to be merged after the ones from filterDOMProps. Is there something I'm missing here as to why this is a separate function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because filterDOMProps isn't a hook so it cannot call other hooks... :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh, right ugh

# Conflicts:
#	packages/@adobe/react-spectrum/src/index.ts
#	packages/@react-spectrum/breadcrumbs/test/Breadcrumbs.test.js
#	packages/@react-spectrum/combobox/test/ComboBox.test.js
#	packages/@react-spectrum/link/test/Link.test.js
#	packages/@react-spectrum/list/test/ListView.test.js
#	packages/@react-spectrum/listbox/test/ListBox.test.js
#	packages/@react-spectrum/picker/test/Picker.test.js
#	packages/@react-types/shared/src/dom.d.ts
@devongovett devongovett dismissed stale reviews from LFDanLu and snowystinger via eaa7491 March 28, 2024 13:49
@rspbot
Copy link

rspbot commented Mar 28, 2024

LFDanLu
LFDanLu previously approved these changes Mar 28, 2024
# Conflicts:
#	packages/@react-aria/listbox/src/useOption.ts
#	packages/@react-aria/menu/src/useMenuItem.ts
@rspbot
Copy link

rspbot commented Apr 2, 2024

@rspbot
Copy link

rspbot commented Apr 3, 2024

@rspbot
Copy link

rspbot commented Apr 3, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-aria/utils

RouterProvider

 RouterProvider {
   children: ReactNode
-  navigate: (string) => void
+  navigate: (Href, RouterOptions | undefined) => void
+  useHref?: (Href) => string
 }

useFormReset

-
+useLinkProps {
+  props: LinkDOMProps
+  returnVal: undefined
+}

@react-stately/selection

MultipleSelectionManager

 MultipleSelectionManager {
   canSelectItem: (Key) => boolean
   childFocusStrategy: FocusStrategy
   clearSelection: () => void
   disabledBehavior: DisabledBehavior
   disabledKeys: Set<Key>
   disallowEmptySelection?: boolean
   extendSelection: (Key) => void
   firstSelectedKey: Key | null
   focusedKey: Key
+  getItemProps: (Key) => any
   isDisabled: (Key) => boolean
   isEmpty: boolean
   isFocused: boolean
   isLink: (Key) => boolean
   isSelected: (Key) => boolean
   isSelectionEqual: (Set<Key>) => boolean
   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, 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
+  getItemProps: (Key) => void
   isDisabled: (Key) => void
   isEmpty: boolean
   isFocused: boolean
   isLink: (Key) => void
   isSelected: (Key) => void
   isSelectionEqual: (Set<Key>) => void
   lastSelectedKey: Key | null
   rawSelection: ISelection
   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
 }
 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links inside RouterProvider with basename have inconsistent href handling
6 participants