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

Add option to disable auto email mailto: links #261

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

Conversation

picatz
Copy link

@picatz picatz commented Sep 18, 2023

This PR aims to fix #260 and #82 by adding an option to disable auto email mailto: links.

@@ -227,6 +227,13 @@ func (tr *ANSIRenderer) NewElement(node ast.Node, source []byte) Element {
u := string(n.URL(source))
label := string(n.Label(source))
if n.AutoLinkType == ast.AutoLinkEmail && !strings.HasPrefix(strings.ToLower(u), "mailto:") {
if tr.context.options.DisableAutoEmailLinks {
return Element{
Copy link
Member

@bashbunni bashbunni Oct 3, 2023

Choose a reason for hiding this comment

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

Hey, thanks for the contribution! This is a feature I know people are interested in.

One thing to keep in mind is this also strips the link styling of anything that looks like an email. Do we want to keep the ability to style these components with custom styles? Would be great to hear your thoughts :)

Copy link
Author

Choose a reason for hiding this comment

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

😄 Thank you for the review!

For my own use case, I would prefer to just remove the styling for anything that looks like an email. But, I can totally see other people's use case where they want to modify the style instead.

Happy to modify this PR if you think we should not just disable email auto links, but allow them to be disabled or otherwise manipulated through a custom style. But, I might need a little more time to get that sorted out.

Copy link
Author

Choose a reason for hiding this comment

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

👋 @bashbunni, just wanted to follow up on this again. Should I try to update this PR to keep the ability to style these components?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @picatz I'm going to leave it open for now, I'm currently doing a big refactor of glamour, so will take a look again once that's done.

Copy link
Author

Choose a reason for hiding this comment

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

Understood, thank you for the update!

@picatz
Copy link
Author

picatz commented Oct 28, 2024

Thank you for approving @caarlos0!

@bashbunni, @aymanbagabas is there anything else needed from this PR?

@aymanbagabas
Copy link
Member

@bashbunni, @aymanbagabas is there anything else needed from this PR?

Hi @picatz, thank you for working on this. Please see my comment here

@picatz
Copy link
Author

picatz commented Oct 28, 2024

Thanks for the quick reply @aymanbagabas. I guess I was a little confused, seeing @caarlos0's approval this morning.

😄 I have two follow up questions:

1. Should this PR be closed in favor of the proposed LinkStyler type in #204 (comment)? Please feel free to do so!

2. Should a new issue be created that captures the LinkStyler idea for #260, #82, #178, and (the other) #204 (comment)?

@caarlos0
Copy link
Member

oh, that's my bad, I hadn't seen that other issue

@picatz
Copy link
Author

picatz commented Oct 28, 2024

No worries @caarlos0, totally understand! 😄

@aymanbagabas
Copy link
Member

1. Should this PR be closed in favor of the proposed LinkStyler type in #204 (comment)? Please feel free to do so!

Yes, once we have a new PR for LinkStyler 🙂

2. Should a new issue be created that captures the LinkStyler idea for #260, #82, #178, and (the other) #204 (comment)?

That would be helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go Modules URI incorrectly parsed as mailto link Wrong rendering assumption when using @ in markdown files
4 participants