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

FIX: respect nofollow exclusion setting in topic featured links. #11858

Merged
merged 6 commits into from Jun 15, 2021

Conversation

vinothkannans
Copy link
Member

Previously, nofollow attribute is not removed even when a domain is added to the exclude_rel_nofollow_domains site setting.

Previously, nofollow attribute is not removed even when a domain is added to `exclude_rel_nofollow_domains` site setting .
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/exclude-rel-nofollow-is-not-enforced-in-title-links/168847/4

@@ -11,6 +11,16 @@ export function addFeaturedLinkMetaDecorator(decorator) {
export function extractLinkMeta(topic) {
const href = topic.get("featured_link");
const target = User.currentProp("external_links_in_new_tab") ? "_blank" : "";
const domain = topic.get("featured_link_root_domain");
let allowList = Discourse.SiteSettings.exclude_rel_nofollow_domains;
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to move away from Discourse.SiteSettings constant usage as it's a global variable.

Instead could you pass in allowList as an option to this function? See this PR didn't add this but the same approach should be done for User.currentProp - we could pass that in as an option externalLinksInNewTab: true when calling the function.

This is necessary for newer versions of Ember and to allow us to upgrade our application.

Copy link
Member Author

Choose a reason for hiding this comment

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

See this PR didn't add this but the same approach should be done for User.currentProp

If I update it then I have to change 3 other files which are not related to this PR. So I will do it in a separate PR.

@tgxworld tgxworld added Changes Requested javascript Pull requests that update Javascript code labels Feb 1, 2021
Copy link

@divine-bot divine-bot left a comment

Choose a reason for hiding this comment

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

Good

Copy link

@divine-bot divine-bot left a comment

Choose a reason for hiding this comment

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

Good

@SamSaffron
Copy link
Member

@vinothkannans can you try removing the Discourse.SiteSettings call this week so we can merge?

@vinothkannans
Copy link
Member Author

Sorry for the delay. It's now updated.
Since the site settings are now available in all the rest models I just retrieved it from the topic argument.

@vinothkannans vinothkannans merged commit 9b200ab into master Jun 15, 2021
@vinothkannans vinothkannans deleted the nofollow-featured-link branch June 15, 2021 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code
6 participants