Make event location links clickable #641

dan-foley · 2025-08-26T17:28:58Z

Description

Added an anchor tag around the events location link making it clickable.

Type of change

Please select everything applicable. Please, do not delete any lines.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
This change requires an update to testing

Issue

Is this related to a specific issue? If so, please specify:

Checklist:

This PR is up to date with the main branch, and merge conflicts have been resolved
I have executed npm run test and npm run test:e2e and all tests have passed successfully or I have included details within my PR on the failure.
I have executed npm run lint and resolved any outstanding errors. Most issues can be solved by executing npm run format
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings

DevinCLane · 2025-08-26T18:26:05Z

looks good on first glance

also in #621 we did specify that events should be clickable in the event description as well. you are welcome to add it to this PR, or if you'd like to split it out into a different PR that's fine too

dan-foley · 2025-08-26T18:50:06Z

Accidentally closed the request lol. I'll try to get links in the description working on this PR too.

dan-foley · 2025-08-26T19:12:36Z

@DevinCLane

It seems like it might be a good idea to use a library called react-linkify

https://www.npmjs.com/package/react-linkify

Is there anything special I need to consider when adding a new library?

DevinCLane · 2025-08-27T23:09:42Z

can you mention in the Discord to see if others have opinions? seems fine for me, but ideally we get 1-2 other sign offs before moving forward

dan-foley · 2025-08-28T18:20:26Z

Links are now working int he event description as well! Thanks to @indifferentghost for the help!

DevinCLane · 2025-08-28T21:57:14Z

client/src/features/modal/EventModal.jsx

+    if (URLRegex.test(res)) {
+      // Prepend http:// if it starts with www. to make a valid href
+      const href = res.startsWith("www.")
+        ? `http://${res}`


Suggested change

? `http://${res}`

? `https://${res}`

i'm thinking we would want to add https instead of just http?

Good catch, I'll try to have this done tomorrow.

We 100% want to always send to https.

20jasper · 2025-08-29T00:03:47Z

client/src/features/modal/EventModal.jsx

+          rel="noopener noreferrer"
+          className="text-blue-600 underline hover:text-blue-800"
+        >
+          {res}


I think it'd be best to show the punycode representation here since unicode can be misleading

Consider the following case. It looks awfully similar to apple.com, but it's not really

new URL("https://аррlе.com") URL { href: "https://xn--l-7sbq6ba.com/", origin: "https://xn--l-7sbq6ba.com", protocol: "https:", username: "", password: "", host: "xn--l-7sbq6ba.com", hostname: "xn--l-7sbq6ba.com", port: "", pathname: "/", search: "" }

indifferentghost

I may have overstepped my suggestions, here's why I think this improves the code:

It makes the second URL check more useful by helping us format values
Parsing a URL through the URL handler ensures data integrity. We are sending a url and not an http:// prepended string.

Here's the cons of this approach:

We're losing the www. from the URL.
The more regex used the harder it is to follow
Named capturing groups are relatively new, anyone using a non-standard or 7+ year old browser may have problems viewing this content.

indifferentghost · 2025-08-28T23:26:55Z

client/src/features/modal/EventModal.jsx

 import { useAuthContext } from "../../contexts/AuthContext";
 import { useEventsContext } from "../../contexts/EventsContext";

+const URLRegex = /(https?:\/\/[^\s]+|www\.[^\s]+)/g;


I'm wondering if we can use capturing groups to better effect:

We can start by setting up capturing groups

const URLRegex = /(https?:\/\/|www\.)([^\s]+)/g;

This attempts to isolate the scheme and www subdomain from the rest of the url. The first capturing group grabs either the protocol https:// or the www. subdomain, but lacks the ability to grab both.

We can add an additional capturing group and make it optional ?.

const URLRegex = /(https?:\/\/(www.)?|www\.)([^\s]+)/g;

Our capturing groups are now a mess, so you can use a non-capturing group to clean things up.

const URLRegex = /(?:https?:\/\/(?:www.)?|www\.)([^\s]+)/g;

But even better might be giving some implicit documentation so we know what we're doing. Named capturing groups also exist.

Suggested change

const URLRegex = /(https?:\/\/[^\s]+|www\.[^\s]+)/g;

const URLRegex = /(https?:\/\/(www.)?|www\.)(?<host>[^\s]+)/g;

This is actually hard to grok just from looking at the regex. It might be worthwhile to throw some jsdoc here.

Yeah I'm a total regex noob so I wasn't sure I was going to do it right lol. I'll try to add some jsdoc once I can understand what is going on myself.

When I add the new regex line the link no longer works. Did you test it yourself?

I'm curious what browser you're running. The [browser compatability](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Named_capturing_group#browser_compatibility) shows it works on modern browsers

Weird, I'm using the latest version of Chrome. I sent you a message on Discord. If you want we can set up a time to meet on there and I can show you the problem I am having.

indifferentghost · 2025-08-28T23:36:39Z

client/src/features/modal/EventModal.jsx

+  const separated = children.split(URLRegex);
+
+  return separated.map((res, index) => {
+    if (URLRegex.test(res)) {


With a named capturing group we can better use our methods to get a predictable result. (I prefer ending early over gate control.

Suggested change

if (URLRegex.test(res)) {

const parsedUrl = URLRegex.exec(res);

if (parsedUrl === null) {

return res;

}

indifferentghost · 2025-08-28T23:40:37Z

client/src/features/modal/EventModal.jsx

+      const href = res.startsWith("www.")
+        ? `http://${res}`
+        : new URL(res).toString();


Finally we add back the schema and parse it. host may not be the correct group name. domain may be more descriptive.

There is some consideration that our www subdomain is removed, but I think that's fine.

Suggested change

const href = res.startsWith("www.")

? `http://${res}`

: new URL(res).toString();

const formattedUrl = `https://${parsedUrl.groups.host}`;

if (!URL.canParse(formattedUrl)) {

throw new Error(`Unable to parse ${res}`);

}

const href = new URL(formattedUrl).toString();

indifferentghost · 2025-08-28T23:47:04Z

client/src/features/modal/EventModal.jsx

+    if (URLRegex.test(res)) {
+      // Prepend http:// if it starts with www. to make a valid href
+      const href = res.startsWith("www.")
+        ? `http://${res}`


We 100% want to always send to https.

indifferentghost · 2025-08-28T23:49:34Z

client/src/features/modal/EventModal.jsx

+            <span>
+              Location:{" "}
+              <a
+                href={modal.activeEvent.location}
+                target="_blank"
+                rel="noopener noreferrer"
+                className="text-blue-600 underline hover:text-blue-800"
+              >
+                {modal.activeEvent.location}
+              </a>
+            </span>


Is this different from the LinkText component we built?

Yes, I did that part first but wasn't able to do the same thing for the event description links. Thats where you came in and recommended the LinkText component.

indifferentghost · 2025-08-28T23:51:21Z

client/src/features/modal/EventModal.jsx

        <div className="description break-words w-auto min-h-20 mb-2 p-2 border-solid border-black border-2 font-semibold rounded-xl bg-neutral-200/50">
-          <p>{modal.activeEvent.description}</p>
+          <p>
+            <LinkText>{modal.activeEvent.description}</LinkText>


If the LinkText isn't for user inserted content, we may have over-engineered, but it's okay.

indifferentghost · 2025-08-28T23:55:21Z

client/src/features/modal/EventModal.jsx

+          {res}
+        </a>
+      );
+    }


Since our check returns res early we don't need this closing bracket.

Suggested change

}

Here's the early returning check #641 (comment)

indifferentghost · 2025-08-29T00:03:54Z

client/src/features/modal/EventModal.jsx

+          rel="noopener noreferrer"
+          className="text-blue-600 underline hover:text-blue-800"
+        >
+          {res}


This should reference the href as display links can be deliberately misleading.

Suggested change

{res}

{href}

dan-foley · 2025-08-29T01:31:06Z

@indifferentghost honestly most of that went over my head, if anyone else can weight in and let me know if those changes all make sense I'll try to implement them.

DevinCLane · 2025-08-30T00:14:16Z

i like these changes and can moderately follow this. seems like it makes sense to implement then continue the review

dan-foley · 2025-09-10T15:16:01Z

I have gone through and made changes, making the regex also handle something like example.com as well as www.example.com and https://www.example.com. I also tried to do the punycode thing @20jasper mentioned and replaced http with https on line 24. Let me know what you think!

DevinCLane · 2025-09-16T18:57:50Z

client/src/features/modal/EventModal.jsx

+        res.startsWith("http://") || res.startsWith("https://")
+          ? res
+          : res.startsWith("www.")
+          ? `https://${res}`
+          : `https://${res}`; // bare domains like google.com


i'm not sure i understand this part, could you walk me through what's happening here?

It checks if res has a protocol of either http or https, if not it checks if it starts with www. Then if it doesn't have any of that it assumes it is a bare domain like if someone just typed google.com. This way the link will always be prepended with https.

I just changed this to an if-then statement for better readability.

DevinCLane · 2025-09-16T19:54:23Z

client/src/features/modal/EventModal.jsx

+          key={index}
+          href={href}
+          target="_blank"
+          rel="noopener noreferrer"


we might not need this if we have target=_blank

https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Attributes/rel/noopener

I removed noopener but kept noreferrer

DevinCLane · 2025-09-30T19:56:40Z

client/src/features/modal/EventModal.jsx

+
+      return (
+        <a
+          key={index}


let's instead use the key={${res}-${index}} as the key value instead of the index

Just updated it!

DevinCLane · 2025-10-04T01:08:34Z

ok i'll see if i can review this in the next week!

client/src/features/modal/EventModal.jsx

dan-foley requested review from DevinCLane, alcpereira, emmebravo, funbunch, intelagense and merrua as code owners August 26, 2025 17:28

dan-foley closed this Aug 26, 2025

dan-foley reopened this Aug 26, 2025

dan-foley force-pushed the Events-display-links-properly branch from 12d53a9 to 0efc32c Compare August 28, 2025 18:17

DevinCLane requested changes Aug 28, 2025

View reviewed changes

20jasper reviewed Aug 29, 2025

View reviewed changes

indifferentghost suggested changes Aug 29, 2025

View reviewed changes

DevinCLane reviewed Sep 16, 2025

View reviewed changes

dan-foley force-pushed the Events-display-links-properly branch from f0dbe8d to ea6d2c7 Compare September 17, 2025 14:19

DevinCLane reviewed Sep 30, 2025

View reviewed changes

Make event location links clickable

cddbd0b

dan-foley force-pushed the Events-display-links-properly branch from ea6d2c7 to cddbd0b Compare October 1, 2025 17:50

dan-foley requested a review from moses-codes as a code owner October 1, 2025 17:50

DevinCLane reviewed Oct 14, 2025

View reviewed changes

client/src/features/modal/EventModal.jsx Show resolved Hide resolved

	const URLRegex = /(https?:\/\/[^\s]+\|www\.[^\s]+)/g;
	const URLRegex = /(https?:\/\/(www.)?\|www\.)(?<host>[^\s]+)/g;

-    if (URLRegex.test(res)) {
+    const parsedUrl = URLRegex.exec(res);
+    if (parsedUrl === null) {
+      return res;
+    }

-      const href = res.startsWith("www.")
-        ? `http://${res}`
-        : new URL(res).toString();
+      const formattedUrl = `https://${parsedUrl.groups.host}`;
+      if (!URL.canParse(formattedUrl)) {
+        throw new Error(`Unable to parse ${res}`);
+      }
+      const href = new URL(formattedUrl).toString();

Make event location links clickable #641

Are you sure you want to change the base?

Make event location links clickable #641

Uh oh!

Conversation

dan-foley commented Aug 26, 2025

Description

Type of change

Issue

Checklist:

Uh oh!

DevinCLane commented Aug 26, 2025

Uh oh!

dan-foley commented Aug 26, 2025

Uh oh!

dan-foley commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DevinCLane commented Aug 27, 2025

Uh oh!

dan-foley commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indifferentghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indifferentghost Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-foley commented Aug 29, 2025

Uh oh!

DevinCLane commented Aug 30, 2025

Uh oh!

dan-foley commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-foley Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dan-foley commented Aug 26, 2025 •

edited

Loading

indifferentghost Sep 3, 2025 •

edited

Loading

dan-foley Sep 17, 2025 •

edited

Loading

DevinCLane Sep 30, 2025 •

edited

Loading