#14: Link click listeners#19
Conversation
| // 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:foo@example.com.
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
| 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
3 participants
No description provided.