ritz078 / embed-js

🌻 A lightweight plugin to embed emojis, media, maps, tweets, code and more. ✨
https://codesandbox.io/s/Wp5OlNMn
MIT License
1.27k stars 88 forks source link

Duplicate tweets #243

Closed AlMcKinlay closed 6 years ago

AlMcKinlay commented 7 years ago

Hey, I know that when you post 2 of the same image, it isn't duplicated, but it seems to duplicate when you have 2 of the same tweet url.

This becomes an issue if my twitter url is already linked (we use another thing to parse the urls, not embed.js) so when we have

  <a href="https://twitter.com/AndroidDev/status/669199553883742208" target="_blank" rel="noopener">https://twitter.com/AndroidDev/status/669199553883742208</a>

you get 2 images.

Example: https://codepen.io/YaManicKill/pen/aWBoax

Thanks for your work.

ritz078 commented 7 years ago

So you do not want to parse URLs to anchor tags ?

AlMcKinlay commented 7 years ago

We have a parser that parses a number of things including urls and other irc related things. I could remove the url parsing from it, but it works really well currently.

So yeah, I have the url parsing turned off in embed.js, as we do it ourselves before passing the element to embed.js, meaning that it gets the duplicate links.

ritz078 commented 7 years ago

I am not getting what you mean with duplicate links. Can you post a screenshot of your app showing what's happening ?

AlMcKinlay commented 7 years ago

Sure.

f3d1cbeeb9b336ffc1e721a54eb2af01

The reason that it is rendering the tweet twice is because I send this to embed.js:

<a href="https://twitter.com/AndroidDev/status/669199553883742208" target="_blank" rel="noopener">
https://twitter.com/AndroidDev/status/669199553883742208
</a>

This has a url that is linked to the same url, meaning that embed.js sees 2 urls (one in href= and one inside the itself). Does that make sense?

ritz078 commented 7 years ago

How about you provide the the custom URL template to embed.js itself ?

AlMcKinlay commented 7 years ago

How would that work? I can't see it in the documentation.

AlMcKinlay commented 7 years ago

Ah wait, do you mean this? I guess we could, but I don't want to just throw out all the work our guys have done with our url parser. I was just wondering if there was a way I could tell embed.js to ignore duplicate links.

ritz078 commented 7 years ago

Yes maybe that's missing in documentation.

You can pass a custom renderer.

const renderer = {
   url (match, options) {
   // write your custom template.
    let config = options.linkOptions
    return `<a href="${toUrl(match)}" rel="${config.rel}" target="${config.target}">${match}</a>`
 }
}

const x = new EmbedJS(options, renderer)

I see that the URL you are passing is same that's processed by embed.js so why parse it separately ?

Currently its not possible but I agree that it should be.

AlMcKinlay commented 7 years ago

Alright, great, thanks.

The reason we don't want to just switch to using your parsing is that we call embed.js only when the user clicks on the button to expand the preview. Using your parsing would mean that the url wouldn't be an anchor element until the user clicks to expand, which obviously wouldn't be ideal.

Thanks for your work.

ritz078 commented 7 years ago

So you want to avoid loading embed.js only after the user clicks because of its size ?

I agree that this one's a bug. I just want to find if there's an immediate fix so that you are not stuck.

AlMcKinlay commented 7 years ago

It's not due to size, it's due to some users only wanting to view some previews, not all. They don't want every link to take up space on their screen, so there is a setting that means it'll only show if they click the button.

ritz078 commented 7 years ago

You can disable those that you don't want to see and enable them when the user clicks on preview.

AlMcKinlay commented 7 years ago

Yeah, but the issue is that the urls won't be anchors in the message until the user clicks on the preview button.

ritz078 commented 7 years ago

They will be.

See this https://codepen.io/ritz078/pen/wdovzm

No services will embed anything. emojis and urls will work.

AlMcKinlay commented 7 years ago

But when I eventually update the config it to show the embed, I will get duplicates because it is now parsing the 2 urls again. It also parses the url again, and gets really confused with formatting, but that's a separate issue.

https://codepen.io/YaManicKill/pen/dWOyOp

Am I missing something? How do I go from what you have to then having the tweet embedded?

ritz078 commented 7 years ago

https://codepen.io/ritz078/pen/EmNaWX

But yes there are improvements that I will need to make to just pass a string and get it rendered in any HTML element. For now, the the above example should work for you.

AlMcKinlay commented 7 years ago

Aha, I hadn't thought about using destroy, thanks that'll work for just now.

ritz078 commented 6 years ago

This should be solved in v5