Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added default value to link's "rel" when link's "target" is "_blank" #761

Closed
wants to merge 1 commit into from

Conversation

galabra
Copy link
Contributor

@galabra galabra commented Aug 24, 2023

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

When a link's target is set to _blank, the consensus is adding rel="noopener noreferrer".
I added this as the default behavior and also allowed overriding this value.

This was discussed in the past: #413 and in the prehistoric #12

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Aug 24, 2023
@wooorm
Copy link
Member

wooorm commented Aug 24, 2023

Did you see the related issues when searching for rel?
#350 or more recently for example #65 (comment).

What is your aversion to using a plugin instead of passing options?

@galabra
Copy link
Contributor Author

galabra commented Aug 24, 2023

@wooorm Woah, I wasn't looking thoroughly enough and didn't see #350.
For simplicity sake, I honestly assumed the consensus and should be the default option here.

@wooorm
Copy link
Member

wooorm commented Aug 24, 2023

It’s an old issue from before plugins worked. They are closed because there is a better alternative.
The defaults are safe: don‘t use _target.
If anything, I’d remove linkTarget. It’s a bad idea anyway; I think it’s better to let users choose where they open things. E.g., https://css-tricks.com/use-target_blank/.

@galabra
Copy link
Contributor Author

galabra commented Aug 24, 2023

I beg to differ 🙂
Installing additional dependencies is never a good thing, but a tradeoff.
What good reason is there to not extend this functionality?

@wooorm
Copy link
Member

wooorm commented Aug 24, 2023

Everything is a trade off. The trade off here is between a small project that everyone uses or a big project with features many people do not use.
Another good reason is that people can already do this: pass an a component.
Another reason is that users will want to do different things, plugins allow that

@galabra
Copy link
Contributor Author

galabra commented Aug 24, 2023

IMO this should be a part of this library, however I understand where you're coming from.
Thank you for the suggestion to create a custom component, I didn't know it existed!

For future generations, this is what I ended up with:

<ReactMarkdown
    components={{
        a: ({
            node, href, children, ...props
        }) => <a href={href} target="_blank" rel="noopener noreferrer" {...props}>{children}</a>,
    }}
>
    {markdownText}
</ReactMarkdown>

@galabra galabra mentioned this pull request Aug 24, 2023
5 tasks
@wooorm
Copy link
Member

wooorm commented Aug 25, 2023

Glad you found something that works, and thanks for the PR!

@wooorm wooorm closed this Aug 25, 2023
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Aug 25, 2023
@github-actions

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Aug 25, 2023
wooorm pushed a commit that referenced this pull request Sep 25, 2023
Closes GH-761.
Closes GH-762.

Reviewed-by: Titus Wormer <tituswormer@gmail.com>
Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com>
Reviewed-by: Remco Haszing <remcohaszing@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

2 participants