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 federated post and comment links. Fixes #569 #575

Merged
merged 2 commits into from
Feb 14, 2022
Merged

Conversation

dessalines
Copy link
Member

No description provided.

@dessalines dessalines enabled auto-merge (squash) February 14, 2022 18:48
@dessalines dessalines merged commit e13ab2e into main Feb 14, 2022
>
<Icon icon="link" classes="icon-inline" />
</Link>
{/* TODO comment ap_ids are currently broken anyway, so use post.ap_id, and wait until comment tree / endpoint refactor */}
Copy link
Member

Choose a reason for hiding this comment

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

This will be pretty confusing. And ap_ids arent broken at all, there is just no frontend route for them. So it should just show the comment ap_id, as thats what you need to search on another platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment ap_ids are broken. This goes nowhere currently: https://lemmy.ml/comment/93927

That's waiting on a proper comment endpoint which we should probably hold off on until I start tree-views.

{!cv.comment.local && (
<a className={classnames} title={title} href={cv.post.ap_id}>
<Icon icon="fedilink" classes="icon-inline" />
</a>
Copy link
Member

Choose a reason for hiding this comment

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

It should probably be shown for local comments/posts as well, otherwise it will also be confusing that some dont have it.

Copy link
Member

@Nutomic Nutomic Feb 14, 2022

Choose a reason for hiding this comment

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

In fact, i dont think it makes sense to show separate icons for link and activitypub link. If you want to share a specific post/comment with someone, it should be using the link where it was originally posted, not on the instance where you happen to browse. So it should show the ap_id in all cases (not sure which icon though).

That means we definitely have to implement a view under /post/123/comment/456, but that should be really easy if its simply an alias for /comment/456.

Copy link
Member Author

@dessalines dessalines Feb 14, 2022

Choose a reason for hiding this comment

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

It should probably be shown for local comments/posts as well, otherwise it will also be confusing that some dont have it.

In that case it'd just be the same link twice, which would be pointless. The clutter will start to pile up on mobile especially.

If you want to share a specific post/comment with someone, it should be using the link where it was originally posted, not on the instance where you happen to browse. So it should show the ap_id in all cases (not sure which icon though).

Not sure abt this one. I could see some cases where you'd want to link a specific instance's cached version of that post, esp if one instance goes down, gets blocked, or you're linking to someone you know doesn't have an account on the original. But I do agree in most cases the federated one for non-locals makes sense. Maybe for federated / non-local content, just don't show the original link?

That means we definitely have to implement a view under /post/123/comment/456, but that should be really easy if its simply an alias for /comment/456

The actual comment ap_id is just instance/comment/number, which doesn't include the post. The /post/number/comment/number is a front end scrolling hack that I'm starting to regret adding, because its really not the right way to refer to comments. I'd rather this just wait on the proper comment endpoint than continue working building on the hack.

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.

2 participants