-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Automatically add a link card for URLs in the composer #3566
Conversation
|
what happens if i press Post while it's attaching? |
@gaearon It gives me this if I try to submit too early - tested on both iOS and browser. Is that not happening for you? |
simplify was paste check use a set simplify the cross platform reuse web implementation remove log pasting in the middle of a block of text proper regex dont re-add immediately after paste and remove don't use `byteIndex` lfg automatically add link card
c47f0d5
to
2e58f5b
Compare
i think it should not ask me to wait, but should get into a "posting..." state and wait for it automatically |
yeah that should be fine. will add in a subsequent PR. going to check why you can't post just a link card on web as well. one thing i noticed too during this was that threads will fall back to some other image on the page if there isn't an og:image tag available, might be something to look at on backend. we have a lot of cards without images on our network. |
This fixes #2321 🙏 This comment in that issue raises a question: as things stand, would editing a URL automatically update the link card? Before generating the link card I also often edit URLs to remove gunk at the end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant work @haileyok, it feels great. My one thing is, on iOS it was giving me a full "app crashed" error for links that dont have an image (https://example.com being one). Android and web seemed fine with it. Not sure what's going on there, so might want to double-check it.
@pfrazee This has been a thing for a while I think. I don't get a crash on prod builds but definitely do on the dev client. Unsure why, I assume because file paths are different (the simulator somehow uses a local directory outside the simulator's environment). Checked on main and I see the same crash there right now. Weird. |
Huh. Okay. Guess we don't need to prioritize it then. |
@@ -92,6 +96,8 @@ export const TextInput = forwardRef(function TextInputImpl( | |||
* @see https://github.com/bluesky-social/social-app/issues/929 | |||
*/ | |||
setTimeout(async () => { | |||
const wasPaste = newText.length > prevLength.current + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more like mayBePaste
right? You could have multiple added characters for other reasons, like maybe accepting an autocomplete suggestion or an autofix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Let me verify that it doesn't actually matter (it shouldn't, since your cursor won't be next to the URL if you're doing an auto correct) 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oky, we are not relying on it being a paste, but going to update the wording to make it more clear here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this feels great, one question above re: naming (i.e. do we rely on it being a paste, or are false positives ok)
@surfdude29 That's something we can probably do, needs a bit of thinking on how to do it. Fwiw, all the requests go through CardyB so you're fine whenever you paste. Do need to just clear the gunk out then re-add though for the moment. |
Why
Because it's easy to forget to add a link card whenever you mean to, we should instead add them as soon as they are typed or pasted into the composer. This mimics the behavior of other apps such as Threads or Twitter.
How
We want to add a link card in the following situations:
Things to note
try { new URL(uri); return true; } catch (e) { return false }
to determine if a link is valid, because something likehttps://facebook
will be considered valid. Instead, we use a little (very big!) regex to determine the validity of a link.pos
value for the cursor one ahead of what React Native keeps it at. In the web implementation, we just subtract 1 from the position to keep the rest of the logic the same.Step One - Determine if it was a paste
This is pretty simple. We know that the text has been pasted if the new text is two or more characters greater than the previous text.
Step Two - Figure out if we want to add a link card yet
Given the logic above about cursor position, we can assume that the link is complete and we should attempt to add a link card if:
wasPaste
variable to detect this.Step Three - Keep track of previously added URIs
If we add a URI and then remove it, we shouldn't add again if you paste it again. Keeping track of this also stops something like this from happening
https://github.com/bluesky-social/social-app/issues/3561
If we didn't keep track of previously added links, we'd end up re-adding this link.
With that said, we do want to be able to add back a link card if we remove the link card and the link from the text entirely. So, we clean up our set of URIs each time we update the rich text.
Step Four - Verify it is a good link
Simple regex check.
Step Five - Add the card
Call
onNewLink(uri)
and add the link to the previously added links.Test Plan
Various test cases:
Paste Input
Don't add it again
But add it again if you completely remove it from the text
Don't add until finished typing
Even if you're adding a path
Remember not to add it back!
And of course, accept a different link if you have removed one, even if it's in the middle of the text
It's definitely possible I missed something here, so we should play with it and see if we run into any strange edge cases.