Skip to content
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

Allow using mailto: links in e-mail buttons #2327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

k-nut
Copy link
Collaborator

@k-nut k-nut commented Nov 6, 2024

Description

Allow using mailto: URIs in e-mails.

Changes

  • Bumps the validator package to the latest version (to get the isMailtoURI function which was added in 13.11.0)
  • Adapts formatURL to leave valid mailto: urls untouched.

Notes to reviewer

I guess it could be argued if formatUrl should be handling mailto URLs like this but I'd argue that a mailto url is still a valid URL. Happy to learn about your differing thoughts though.

Related issues

Resolves #2326

@richardolsson
Copy link
Member

Welcome @k-nut! What a pleasure to see a PR fixing this so quickly after I filed the issue. Great work! 👏

I think the fix is very neat. The only thing I would ask (and hopefully you don't mind me asking) is if you can tell me whether there are other places where formatURL() is used, and whether you have tested those areas to make sure this change makes sense for them?

@k-nut
Copy link
Collaborator Author

k-nut commented Nov 10, 2024

Hi @richardolsson thank you for the nice words. I have to say that the issue was written as detailed and helpful as I have rarely seen it in other projects which made it really easy to jump into. Big props to you!

Regarding your question:
I changed the version of formatURL in src/features/emails/components/EmailEditor/EmailSettings/utils/formatUrl.ts (there exists another in src/utils/formatUrl.ts which actually was identical to the there other one until my change - not sure if this was deliberate?). In any case, the version I changed is used in ButtonBlockListItem.tsx which is the change that I intended to make. It is also used in the InlineLink in e-mails where I would think that it also makes sense.

Let me know if you think that there is something else that I missed!

@richardolsson
Copy link
Member

Regarding your question: I changed the version of formatURL in src/features/emails/components/EmailEditor/EmailSettings/utils/formatUrl.ts (there exists another in src/utils/formatUrl.ts which actually was identical to the there other one until my change - not sure if this was deliberate?).

I'm honestly not sure why there are two, and I'm not in a position to investigate more closely right now. But in this case I think it's not a problem but actually just means that there's less to think about for this change.

In any case, the version I changed is used in ButtonBlockListItem.tsx which is the change that I intended to make. It is also used in the InlineLink in e-mails where I would think that it also makes sense.

Perfect, that answers my question!

With that I would be ready to merge, if it wasn't because I realized that this needs to also be handled in the backend, where mailto: links will need to be handled differently than http*:// links. So I will file an internal issue for that, and will return to this once that's been fixed.

@richardolsson richardolsson added the 🛑 potentially blocked Potentially blocked by prerequisites (double check or ask someone, as it may have changed) label Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛑 potentially blocked Potentially blocked by prerequisites (double check or ask someone, as it may have changed)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow mailto link in email (button or link)
2 participants