#14: Link click listeners #19
Conversation
src/client.js
Outdated
| // Check if href is a relative URL | ||
| const isRelative = href && | ||
| (href.startsWith("./") || href.startsWith("/") || | ||
| !/^(?:[a-z]+:)?\/\//i.test(href)); |
There was a problem hiding this comment.
this last regex probably could be simpler and just be http / https right? Is there some other protocol we need to match for?
There was a problem hiding this comment.
It makes sense to check for more than just http, e.g. tel:3035559999or mailto:[email protected].
But, I agree that we could simplify. According to MDN, the href should already be parsed by the browser, so this might be all we need:
const isExternal = !href.startsWith(appDiv.baseURI);
if (isExternal) return;(using appDiv.baseURI is intentional)
If relative href attributes are not already expanded to absolute URLs, we can normalize it ourselves with the browser's help:
const normalizedHREF = new URL(href, anchor.baseURI).href;https://developer.mozilla.org/en-US/docs/Web/API/Node/baseURI
gabesullice
left a comment
gabesullice
left a comment
There was a problem hiding this comment.
Looking good so far, let's:
- Account for a couple other cases when a link shouldn't be following automatically (see
downloadandtargetcomment below). - Try to simplify the
isRelativecheck a bit
src/client.js
Outdated
| event.stopPropagation(); | ||
|
|
||
| // Call the client's follow function | ||
| client.follow(href, {}); |
There was a problem hiding this comment.
nit: no need to pass the default value IMO
| client.follow(href, {}); | |
| client.follow(href); |
|
@gabesullice @zrpnr Thank you both for your comments and suggestions. I've incorporated all your feedback as best I could. Please let me know if there's anything I missed or should improve further. |
gabesullice
left a comment
gabesullice
left a comment
There was a problem hiding this comment.
This looks good to me! Thanks @kostiukevych-ls :)
Before I merge it though, let's make a draft PR against the https://github.com/Applura/intro repo: Applura/intro#19
| // Check if anchor has a target attribute that is not _blank | ||
| if ( | ||
| anchor.hasAttribute("target") && | ||
| anchor.getAttribute("target") !== "_blank" |
| if (!(event.target instanceof HTMLAnchorElement)) return; | ||
|
|
||
| const anchor = event.target; |
There was a problem hiding this comment.
This is too strict, for example the markup could be something like <a href=""><div class="title">Click me</div></a> Then the event.target would be the div.title instead of the anchor and this will return.
| if (!(event.target instanceof HTMLAnchorElement)) return; | |
| const anchor = event.target; | |
| const anchor = event.target instanceof HTMLAnchorElement ? event.target : event.target.closest('a'); | |
| if (!anchor) return; |
| if (!(event.target instanceof HTMLAnchorElement)) return; | ||
|
|
||
| const anchor = event.target; | ||
| const href = anchor.href; // Safe to access href directly |
There was a problem hiding this comment.
the href property seems to always be absolute, let's use
| const href = anchor.href; // Safe to access href directly | |
| const href = anchor.getAttribute("href"); |
| const normalizedHREF = url.href; | ||
|
|
||
| // Call the client's follow function | ||
| client.follow(normalizedHREF); |
There was a problem hiding this comment.
here normalizedHREF is now the app baseuri and not the api baseURI so follow will return early.
I think it's safe to just use the href here since we have already checked for the protocol and isExternal
| client.follow(normalizedHREF); | |
| client.follow(href); |
| return; | ||
| } | ||
| const isExternal = url.protocol !== "http:" && url.protocol !== "https:" || | ||
| url.origin !== new URL(appDiv.baseURI).origin; |
There was a problem hiding this comment.
Not critical to change now but this could just be isKnownProtocol since we also do the isExternal check by comparing the origins inside follow
No description provided.