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

Even more link handling! #1200

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Mar 13, 2024

Pull Request Description

This PR fixes an issue where non-Lemmy links may incorrectly be identified as such. The fix is, after parsing the link, check whether the instance is a known Lemmy instance. If so, navigate immediately. If not (e.g., if it is federated from another platform), retrieve the object before navigating to ensure that it is a valid object. If we cannot retrieve the object, we will fallback to the web browser. Note that this only done in cases when the "link" is a valid URL. If not (i.e., if it's just "Lemmy syntax"), then it would be pointless to fall back to the browser.

Issue Being Fixed

Issue Number: #1187 (comment)

Screenshots / Recordings

Demo 1: Shows that the original issue is fixed

links_1.mp4

Demo 2: Shows that valid Lemmy links from non-Lemmy software still work (with a delay)

links_2.mp4

Demo 3: Shows that valid links from Lemmy software still work (no delay)

links_3.mp4

Checklist

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

@hjiangsu
Copy link
Member

I think that looks good! Do you think we can add a snackbar, or some UX feedback (spinning indicator) when we're attempting to fetch the object?

@micahmo
Copy link
Member Author

micahmo commented Mar 14, 2024

Do you think we can add a snackbar, or some UX feedback (spinning indicator) when we're attempting to fetch the object?

Hmm good idea. I tried a snackbar first, but it actually took a little while to appear, so it still feels like there's nothing happening for a bit. I also couldn't find a great way to hide it when the page does eventually load (I think you had the same problem).

My next idea was to show a loading page when we have to retrieve the object so it looks like we've navigated immediately. The trick to making this seamless is to hide the page and show the "real" page without any additional animations. For that I used pushReplacement with no transitionDuration and it seems to work well! It's not completely seamless when going to the browser (when not using the in-app one), but I think it's good enough!

Here's a before and after so you can see the time differences.

Before

qemu-system-x86_64_QJCJd1I7co.mp4

After

qemu-system-x86_64_lVSPEx2yQ8.mp4

Here's also what it looks like in the other two browser modes.

In-app

qemu-system-x86_64_SBF2NOZtUb.mp4

External

qemu-system-x86_64_i5E30cRWwE.mp4

I pushed these changes, so this should be good to go if you like it.

@hjiangsu
Copy link
Member

I think that looks good! Just one more question - does the loading happen for all external URLs (e.g., if we press a link on a post, will the brief loading page show up?)

@micahmo
Copy link
Member Author

micahmo commented Mar 14, 2024

does the loading happen for all external URLs (e.g., if we press a link on a post, will the brief loading page show up?)

No, it should only happen when we have to manually load an object, which is only for community/user links when we don't recognize the instance (like the first two examples in the demo).

https://github.com/micahmo/thunder/blob/555d24b8b75c933b19bd6a2d0ca9d9254604a0d4/lib/utils/links.dart#L233

https://github.com/micahmo/thunder/blob/555d24b8b75c933b19bd6a2d0ca9d9254604a0d4/lib/utils/links.dart#L262

@hjiangsu
Copy link
Member

Ahh okay! In that case, I think this works 😄

@hjiangsu hjiangsu merged commit 00bb1cb into thunder-app:develop Mar 14, 2024
1 check passed
@micahmo micahmo deleted the feature/improve-link-handling branch March 15, 2024 02:36
@micahmo micahmo mentioned this pull request Apr 17, 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