-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix synthetic links not using useHref
#6346
Fix synthetic links not using useHref
#6346
Conversation
fba1ec8
to
33ca102
Compare
9fc618c
to
c0a5c54
Compare
c0a5c54
to
75ab16e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this makes sense.
return { | ||
'data-href': props.href, | ||
'data-href': props.href ? router.useHref(props.href) : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could cause issues due to a hook being called conditionally. If we're sure that there is always an href when this function is called, we could probably make href required in the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this can cause issues, however I directly mirrored the implementation of useLinkProps
:
react-spectrum/packages/@react-aria/utils/src/openLink.tsx
Lines 160 to 170 in 10bac54
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 | |
}; | |
} |
Also useSyntheticLinkProps
is used in useGridListItem
, where we do not always have an href.
If we want to prevent useHref
from being called conditionally we would have to instead split up useGridListItem
in to two hooks, one with href and one without. We'd also have to conditionally render between two components everywhere useGridListItem
is used to swap between the two new hooks.
Alternatively we could call useHref
with an empty string. After a quick search this seems to be a valid href according to this detailed stackoverflow comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok if it was there before I guess it's probably fine for now.
Closes #6347
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: