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

More link handling improvements #1187

Merged

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Mar 11, 2024

Pull Request Description

This PR is similar to #1014 but for users rather than communities (now that they are sharing the same feed).

I discovered a link that Thunder was accidentally identifying as a user, when it was actually a community, which led me to discover that the user error handling is a bit clunky. First, I fixed that so it works a little better. Then I fixed the link parsing to see that it's actually a community link.

Review without whitespace.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Original behavior

link_before.mp4

After user error handling improvements

link_middle.mp4

After link parsing improvements

link_after.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

LGTM! Nice catch!

@hjiangsu hjiangsu merged commit 5e61e18 into thunder-app:develop Mar 11, 2024
1 check passed
@micahmo micahmo deleted the feature/more-link-handling-improvements branch March 11, 2024 20:23
@hjiangsu
Copy link
Member

I found one link issue that might be related to all of this: https://lemmy.world/post/13037417 - it tries to find a community but its a link to a mastodon post I believe.

@micahmo
Copy link
Member Author

micahmo commented Mar 12, 2024

Thanks, I'll take a look! I guess the ideal solution would be to fall back to a web browser if we can't navigate natively. This could be fixed by trying to retrieve the object before navigating, but that would add a delay. The current behavior is nice in that the navigation happens immediately, and then we try to load. So we might need to have some way of propagating the error back to the previous page so we can close the community/user feed and then open the browser.

@hjiangsu
Copy link
Member

Yeah, I agree - I do like that the navigation happens immediately (rather than having a delay) for UX purposes. Would it be possible to do something like this:

  • If we detect a user/community link, we can look through our instance list first to see if the instance is available there. If the instance is there, then we know its a proper Lemmy instance and do what we do now (with immediate navigation). If we want to, we can decrease the sensitivity for the instance ci workflow (from active_users_monthly > 50 to active_users_monthly > 25)
  • If the instance is not in our list, then we'll try to retrieve the object, but add a snackbar to show that we're doing something so that the user doesn't try to tap on the link again

@micahmo
Copy link
Member Author

micahmo commented Mar 12, 2024

That seems like a pretty good compromise! Just keep in mind that users/communities from non-Lemmy software will cause a delay, even if the navigation ultimately works.

@hjiangsu
Copy link
Member

Just keep in mind that users/communities from non-Lemmy software will cause a delay, even if the navigation ultimately works.

Yeah. Another possible solution is to keep what we have currently, but show a button on the empty navigated page to open the link in the browser if we fail to resolve the user/community! I'll leave it up to you on which method you prefer (I'm good with either or, or an alternative if you have one)

@micahmo micahmo mentioned this pull request Mar 13, 2024
3 tasks
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