feat: bypass Link click handler if alt click or defaultPrevented #1249
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
I think this is a draft until we create an e2e test. |
rmarscher
force-pushed
the
feat/link-allow-new-tab
branch
from
ed3d6aa to
08e2b4d
Compare
packages/waku/src/router/client.ts
Outdated
| if (!isAltClick) { | ||
| event.preventDefault(); | ||
| } |
There was a problem hiding this comment.
Why can it be this?
if (isAltClick) {
return;
}There was a problem hiding this comment.
That skips the onClick prop. What do you think about firing the onClick prop first? React Router does that and then doesn't run the internal handler if the onClick calls preventDefault().
https://github.com/remix-run/react-router/blob/react-router%407.2.0/packages/react-router/lib/dom/lib.tsx#L628-L635
function handleClick(
event: React.MouseEvent<HTMLAnchorElement, MouseEvent>
) {
if (onClick) onClick(event);
if (!event.defaultPrevented) {
internalOnClick(event);
}
}There was a problem hiding this comment.
Oh, that looks great. Any drawbacks?
There was a problem hiding this comment.
Not that I can think of. It looks good to me. I'm traveling for the next few days so I probably won't push another update until the beginning of next week.
There was a problem hiding this comment.
The last two weeks flew by 😅 But yes, I was also just looking at the open issues and would like to get all of these loose ends wrapped up asap.
rmarscher
force-pushed
the
feat/link-allow-new-tab
branch
from
08e2b4d to
1bcc4b4
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
rmarscher
force-pushed
the
feat/link-allow-new-tab
branch
from
1bcc4b4 to
7d85d80
Compare
rmarscher
changed the title
packages/waku/src/router/client.ts
Outdated
| @@ -81,6 +81,13 @@ const parseRouteFromLocation = (): RouteProps => { | |||
| return parseRoute(new URL(window.location.href)); | |||
| }; | |||
|
|
|||
| function isAltClick(event: MouseEvent<HTMLAnchorElement>) { | |||
There was a problem hiding this comment.
Can you make it an arrow function? (Just a preference)
packages/waku/src/router/client.ts
Outdated
| @@ -265,8 +272,7 @@ export function Link({ | |||
| }; | |||
| } | |||
| }, [unstable_prefetchOnView, router, to]); | |||
| const onClick = (event: MouseEvent<HTMLAnchorElement>) => { | |||
| event.preventDefault(); | |||
| const internalOnClick = (_event: MouseEvent<HTMLAnchorElement>) => { | |||
There was a problem hiding this comment.
It's not confusing without it.
| const internalOnClick = (_event: MouseEvent<HTMLAnchorElement>) => { | |
| const internalOnClick = () => { |
| @@ -265,8 +272,7 @@ export function Link({ | |||
| }; | |||
| } | |||
| }, [unstable_prefetchOnView, router, to]); | |||
| const onClick = (event: MouseEvent<HTMLAnchorElement>) => { | |||
| event.preventDefault(); | |||
There was a problem hiding this comment.
Removing this causes e2e tests to fail, doesn't it?
There was a problem hiding this comment.
Yes, that was a mistake. I thought I moved it, not deleted.
Fixes #1248.