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

Clickable forum mentions #4845

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

Conversation

Samq64
Copy link
Member

@Samq64 Samq64 commented Jul 17, 2022

Resolves #1445

Changes

Adds a new addon that turns @mentions on the forums into clickable links.

Reason for changes

Clickable links are always nice.

Tests

Tested on Chromium 115. Mentions in BBCode tags don't work right now.

@Weredime Weredime added new addon Related to new addons to this extension. `scope: addons` should still be added. type: enhancement New feature for the project scope: addon Related to one or multiple addons and removed new addon Related to new addons to this extension. `scope: addons` should still be added. type: enhancement New feature for the project labels Jul 19, 2022
RedGuy12
RedGuy12 previously approved these changes Jul 19, 2022
@Secret-chest
Copy link
Contributor

This should be a separate addon, more links is for website

@mxmou
Copy link
Member

mxmou commented Jul 20, 2022

This should be a separate addon, more links is for website

Forums are a part of the website.

@DNin01
Copy link
Member

DNin01 commented Jul 22, 2022

This should be a separate addon, more links is for website

This new feature does seem to have a pretty different purpose from the addon it's being added to here, plus, curator-link is a separate addon that also makes a username a link. I kind of agree.

Copy link
Member

@DNin01 DNin01 left a comment

Choose a reason for hiding this comment

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

I see this addon differently and I'm not sure it makes sense to add this feature to this addon. This feature might be better off in a new one instead...? This all depends on what we want this addon to be, though. Maybe all of this is better when it's packed into one more general addon, but I'm not sure what to do.

Copy link
Member

@WorldLanguages WorldLanguages left a comment

Choose a reason for hiding this comment

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

Blocking merge

@WorldLanguages
Copy link
Member

I think this could be a separate addon.
Any objections? @mxmou @Hans5958 @RedGuy12 @Weredime

@Hans5958
Copy link
Member

Hans5958 commented Jul 26, 2022

I'm with Samq64's side and thinks that it is better of to keep it as one addon of more-links.

@Hans5958
Copy link
Member

In retrospect then we should merge curator-links and put it as a checkbox inside more-links.

@WorldLanguages
Copy link
Member

In retrospect then we should merge curator-links and put it as a checkbox inside more-links.

Yeah, if we merge these 3 addons into one, then more-links would need to have 3 checkboxes.

@Hans5958
Copy link
Member

Still better in my opinion due to how relatively small the addons are

@Secret-chest
Copy link
Contributor

Remember the better-quoter and forum-quote-beautifier merge idea? They did completely different things to quotes. This does completely different things to links.

@Hans5958
Copy link
Member

Hans5958 commented Jul 26, 2022

No?

better-quoter changes how the "Quote" button works, forum-quote-code-beautifier changes how code blocks are styled, a style change.

All more-links, this expansion, and curator-links have something in common: creating links from text that are not linked before, and I fail to see how it "does completely different things to links"

Also, please read #4882 (comment) for my intention related to the name.

@Samq64
Copy link
Member Author

Samq64 commented Jul 26, 2022

I think this could be a separate addon.

Should I push that then?

addons/forum-mentions/addon.json Outdated Show resolved Hide resolved
Co-authored-by: Weredime <100447465+Weredime@users.noreply.github.com>
@WorldLanguages
Copy link
Member

I believe this PR has some discussion on it about whether this feature should be merged with other addons.

We're probably going to add this as a separate addon, and use the upcoming "related addons"/"see also" functionality so that all addons link to each other.

@Samq64 Samq64 requested a review from DNin01 August 2, 2023 14:56
@WorldLanguages WorldLanguages modified the milestones: v1.34.0, v1.35.0 Aug 2, 2023
@Samq64 Samq64 added new addon Related to new addons to this extension. `scope: addons` should still be added. and removed status: needs review [Use when there's already 1 approval] Review needed on the PR labels Sep 11, 2023
Copy link
Member

@DNin01 DNin01 left a comment

Choose a reason for hiding this comment

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

We're probably going to add this as a separate addon, and use the upcoming "related addons"/"see also" functionality so that all addons link to each other.

I'm torn on whether to add this to more-links or its own addon, but if we're linking them together, I guess this is fine.

Actually, I think I'm leaning towards a new addon, which is where we are now, but I haven't come up with a final opinion yet. These addons have good reasons to be combined and separate.

@WorldLanguages WorldLanguages modified the milestones: v1.35.0, v1.36.0 Sep 24, 2023
@BroJac5246
Copy link
Contributor

We're probably going to add this as a separate addon, and use the upcoming "related addons"/"see also" functionality so that all addons link to each other.

I'm torn on whether to add this to more-links or its own addon, but if we're linking them together, I guess this is fine.

The point of more-links is making more things links, which is what this does. So doesn't it make more sense for it to be part of that?

@DNin01
Copy link
Member

DNin01 commented Sep 24, 2023

We're probably going to add this as a separate addon, and use the upcoming "related addons"/"see also" functionality so that all addons link to each other.

I'm torn on whether to add this to more-links or its own addon, but if we're linking them together, I guess this is fine.

The point of more-links is making more things links, which is what this does. So doesn't it make more sense for it to be part of that?

It's just more-links and this addon have somewhat different use cases, and are the difference between going to an external site and going somewhere else in Scratch.

@BroJac5246
Copy link
Contributor

We're probably going to add this as a separate addon, and use the upcoming "related addons"/"see also" functionality so that all addons link to each other.

I'm torn on whether to add this to more-links or its own addon, but if we're linking them together, I guess this is fine.

The point of more-links is making more things links, which is what this does. So doesn't it make more sense for it to be part of that?

It's just more-links and this addon have somewhat different use cases, and are the difference between going to an external site and going somewhere else in Scratch.

We need a solution for stuff like this because it has come up in the past...

Good point, though, I agree now.

@Samq64 Samq64 added the status: needs review [Use when there's already 1 approval] Review needed on the PR label Sep 25, 2023
@Hans5958
Copy link
Member

Hans5958 commented Dec 8, 2023

It's just more-links and this addon have somewhat different use cases, and are the difference between going to an external site and going somewhere else in Scratch.

In regards to my vision, I don't see making a distiction between external links and internal links reasonable.

The addon converts any URL that are not linked to a linked URL, with no attention to what the URL leads to. Even if, somehow, Scratch makes links of scratch.mit.edu not being linked, it would get caught by the addon nonetheless. This process has no exceptions, in regards to the RegEx pattern.

In case I haven't mentioned it, this baseless distiction would just complicate things more than ever. Why even bother?

@Joeclinton1
Copy link
Contributor

Is there a reason you identified bbcode as an edge case then just didn't solve it? Also perhaps this addon could be made larger into an addon about making all mentions on all parts clickable, following #7301 and #7364.

@Samq64
Copy link
Member Author

Samq64 commented Jun 21, 2024

Is there a reason you identified bbcode as an edge case then just didn't solve it?

I'll look into it a little later.

Also perhaps this addon could be made larger into an addon about making all mentions on all parts clickable, following #7301 and #7364.

I agree. External links should be one addon and usernames another, but there there's never been a conclusion on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new addon Related to new addons to this extension. `scope: addons` should still be added. scope: addon Related to one or multiple addons status: needs review [Use when there's already 1 approval] Review needed on the PR type: enhancement New feature for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More links addon: Linkify @mentions on the forum
10 participants