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

[EuiMarkdown] Allow mailto: links by default & allow customization of link validation #5790

Merged
merged 13 commits into from
May 7, 2022

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Apr 13, 2022

Summary

Fixes #5770 by adding mailto: markdown links to the list of allowed protocols (now http:, https:, and mailto:), and made the validation customizable.

When a link protocol is disallowed, the link will render as the markdown syntax so the URL and its display text are still present. The exception to this rule is if mailto: is disabled, only the link text will be displayed.

I also took this opportunity to add a documentation section to explain the various plugin defaults used by the markdown components, and extended the existing markdown link validation to accept a configuration object, and documented that 😅.

Also took the time the update isDomainSecure as we can now use the URL method (bye ie11!), avoiding a nasty regex.

Checklist

- [ ] Checked in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles

  • Added documentation
    - [ ] Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
    - [ ] Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@chandlerprall chandlerprall force-pushed the bug/5770-markdown-mailto branch from f7872ac to a58ede3 Compare April 13, 2022 16:36
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5790/

1 similar comment
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5790/

@chandlerprall chandlerprall changed the title [EuiMarkdownEditor] Transform mailto: links back to plain text emails [EuiMarkdownEditor] Remove mailto: links from markdown Apr 13, 2022
@chandlerprall chandlerprall requested a review from cee-chen April 13, 2022 17:40
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5790/

@cchaos cchaos requested a review from elizabetdev April 13, 2022 18:56
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @chandlerprall for fixing this issue.

IMO the links should be presented as links. Not as normal text. We say that we follow the GitHub flavoured markdown:

Screenshot 2022-04-14 at 11 34 36

In Github, if we pass helpdesk@elastic.co or [helpdesk@elastic.co](mailto:helpdesk@elastic.co) both creates a link:

gh-mail-links@2x

But I think we can be a little bit different from Github here. But my expectation as a user is when I pass a link syntax [](url) a link is actually created. Why would I have so much work to try to create a link for nothing?

So if I pass helpdesk@elastic.co it's ok to be normal text. But [helpdesk@elastic.co](mailto:helpdesk@elastic.co) should be a link. If consumers don't want to create link they just pass the text without a link syntax.

mail-links@2x

@chandlerprall chandlerprall force-pushed the bug/5770-markdown-mailto branch from b63fca8 to fdac6a1 Compare May 5, 2022 21:28
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5790/

@@ -6,19 +6,20 @@
* Side Public License, v 1.
*/

const isElasticDomain = /(https?:\/\/(.+?\.)?elastic\.co((\/|\?)[A-Za-z0-9\-\._~:\/\?#\[\]@!$&'\(\)\*\+,;\=]*)?)/g;
const isElasticHost = /^([a-zA-Z0-9]+\.)*elastic\.co$/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful cleanup! 🎉

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5790/

@chandlerprall chandlerprall changed the title [EuiMarkdownEditor] Remove mailto: links from markdown [EuiMarkdown] Allow mailto: links by default & allow customization of link validation May 6, 2022
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5790/

@@ -170,6 +173,186 @@ export const MarkdownPluginExample = {
),

sections: [
{
title: 'Default plugins',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new documentation is fantastic!! Thank you for adding it!

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic. Since Chandler is on PTO starting next Monday and it's late there for him (apologies this took me so long to circle back to this), I'm going to go ahead and commit the suggested changelog request and turn on auto merge.

@cee-chen cee-chen enabled auto-merge (squash) May 7, 2022 01:48
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5790/

@cee-chen cee-chen merged commit 47e869e into elastic:main May 7, 2022
@chandlerprall chandlerprall deleted the bug/5770-markdown-mailto branch June 1, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Markdown Editor] Rendering typed email addresses with mailto, but doesn't convert to link
4 participants