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.

Sign up for GitHub

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

fix: routerized links #4628

Draft
wants to merge 2 commits into
base: canary
Choose a base branch
from
Draft

fix: routerized links #4628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a8f74b
use routerized href to navigate if we are using the router
jprosevear Jan 23, 2025
9f13a25
complete href in the link prop
jprosevear Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/components/link/src/use-link.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export function useLink(originalProps: UseLinkProps) {
as,
children,
anchorIcon,
href,
isExternal = false,
showAnchorIcon = false,
autoFocus = false,
Expand All @@ -73,6 +74,7 @@ export function useLink(originalProps: UseLinkProps) {
const {linkProps} = useAriaLink(
{
...otherProps,
href,
Copy link
Member

Choose a reason for hiding this comment

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

href originally is in otherProps

Copy link
Author

Choose a reason for hiding this comment

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

I know; see below.

onPress,
onPressStart,
onPressEnd,
Expand Down Expand Up @@ -110,6 +112,8 @@ export function useLink(originalProps: UseLinkProps) {
"data-focus": dataAttr(isFocused),
"data-disabled": dataAttr(originalProps.isDisabled),
"data-focus-visible": dataAttr(isFocusVisible),
// The `href` prop may be routerized by useAriaLink, so we may merge over top of it
href,
Copy link
Author

Choose a reason for hiding this comment

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

This makes sure that the href on the is set properly

Copy link
Member

Choose a reason for hiding this comment

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

href from linkProps in L117 will override this href in L116 so all changes done in this file don't really do anything.

p.s. href from linkProps in L117 is routerized, i.e. 1 -> /blogs/1, which is expected. What's the reason to put prop.href here?

Copy link
Author

Choose a reason for hiding this comment

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

In case you are not using a routerized link you still need the original href I thought

Copy link
Member

Choose a reason for hiding this comment

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

href will be passed to useLinkProps, and eventually it will be in linkProps anyway.

Copy link
Author

@jprosevear jprosevear Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It still needs to be extracted from otherProps though so mergeProps doesn't overwrite the linkProps href with the otherProps href.

Copy link
Author

Choose a reason for hiding this comment

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

the key for me is that useLinkProps can change href

...mergeProps(focusProps, linkProps, otherProps),
};
}, [classNames, isFocused, isFocusVisible, focusProps, linkProps, otherProps]);
Expand Down
4 changes: 2 additions & 2 deletions packages/hooks/use-aria-link/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,10 @@ export function useAriaLink(props: AriaLinkOptions, ref: RefObject<FocusableElem
// If props are applied to a router Link component, it may have already prevented default.
!e.isDefaultPrevented() &&
shouldClientNavigate(e.currentTarget, e) &&
props.href
routerLinkProps.href
) {
e.preventDefault();
router.open(e.currentTarget, e, props.href, props.routerOptions);
router.open(e.currentTarget, e, routerLinkProps.href, props.routerOptions);
Copy link
Author

Choose a reason for hiding this comment

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

This makes sure that we use the useHref'ed href for the navigation

Copy link
Member

Choose a reason for hiding this comment

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

by doing so, one couldn't do any absolute paths, e.g. says I'm at /blogs and if i set href to /foo/1, I will be navigated to /blogs//foo/1 instead of /foo/1.

Copy link
Author

Choose a reason for hiding this comment

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

Why not? useHref from RR should produce /foo/1 in that case. I updated https://stackblitz.com/edit/wfdcu9f2 with that case.

Copy link
Member

Choose a reason for hiding this comment

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

RRLink can navigate to /foo/1correctly. With your PR change, I got //blogs/foo/1. Note that even I'm at /blogs originally, it would still add the slash causing // at the end. You may link those components locally to play around.

Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/adobe/react-spectrum/blob/016590a4afc585eea18d5a12853c9d1e2d82a19c/packages/%40react-aria/utils/src/openLink.tsx#L174 for reference - unless the useLinkProps href is used, why call it it at all? Also, if the href is something like "https://www.heroui.com/" i believe the router.open call won't be made on it, a regular link will happen.

Copy link
Author

Choose a reason for hiding this comment

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

RRLink can navigate to /foo/1correctly. With your PR change, I got //blogs/foo/1. Note that even I'm at /blogs originally, it would still add the slash causing // at the end. You may link those components locally to play around.

Can you give more details? All this patch really does is ensure the href produced by useLinkProps from react-aria is actually used in Hero (at least that was my intention)

}
},
}),
Expand Down