New block: Link Preview #47765
New block: Link Preview #47765
Conversation
ellatrix
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +1.96 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1db7e0c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4252162003
|
|
Nice PR, from the screenshots these look close. I'd add the 2px border radius and maybe a little padding. I'm having some trouble reproducing, though. When I paste a URL on an empty paragraph and press Enter, I would expect the link preview to be used, but it's using a regular Embed block:
This looks a new block, "Link preview": That seems okay if it's an embed variation, categorized with the other embeds. But key here would be that it's automatically transformed into, when OG tags are detected. Can we do that? And if so, what's a good URL to test? I'm not sure why but I'm having trouble finding good examples that work:
|
Yes, I mentioned in the PR description that I'd like it to be possible to paste a URL and get a preview, but we need to figure out how it interacts with the embed block because right now we try embedding first.
What do you mean? I think we are checking OG tags now though the
It's good that you find examples that don't work. Could you share them? Cause ideally any link should work. If there's no OG tags, then we can fall back to the document title. |
🚀
Same as above: that it tries to convert it into your new block if there's content to show in it.
I actually tried pasting in a couple of Gutenberg PRs, such as this one. I also tried a couple of wikipedia links, such as
|
But are you trying it with the Link Preview block? The image you shared is the Embed block? |
|
My apologies, I should've included that. Nothing happens, no console error, I click embed but it's stuck:
|
|
Nice PR. I particularly like the idea of auto transforming pasting links into this block 👍 I tested the The REST API endpoint itself has several methods for parsing the various url details. You can see these starting at Most of the methods look for OG tags as well as standard HTML tags. There are certain sites which is doesn't play well with, including sites that rely on JavaScript for rendering and which also don't provide useful metadata in the server response. I don't see any way we can work around that. Google et al. presumably do it by relying on their internal search APIs to retrieve data parser by their own crawlers. We can't do that unfortunately... |
|
This is so strange. I had it working properly when I created the PR, and now the |
|
@joen My apologies as well, I accidentally added an onClick listener that prevents default behaviour on the whole block, so that's why the button in the placeholder didn't work anymore. 🙈 |
There was a problem hiding this comment.
I'd like a way to edit my link after I've submitted it. Currently you can't do that.
Also I tried pasting in a YT watch link like https://www.youtube.com/watch?v=0mUrwyyw-8I&t=269s. The preview was broken (because many elements of data are expected in the block but missing in the response) but the strangest thing was when you click on the link it tries to load internally within the iframe.
Screen.Capture.on.2023-02-08.at.18-37-58.mp4
|
@getdave I added URL editing and fixed it for cases where some of the fields is empty. |
|
Does anyone know how to get the stylesheet enqueued on the front-end? |
ellatrix
added
New Block
| { image && <img src={ image } alt={ title } /> } | ||
| <div> | ||
| { title && <strong>{ title }</strong> } | ||
| { icon && ( | ||
| <img | ||
| className="link-preview__icon" | ||
| src={ icon } | ||
| alt={ new URL( url ).host } | ||
| /> | ||
| ) } | ||
| { title ? new URL( url ).host.replace( /^www\./, '' ) : url } | ||
| </div> |
There was a problem hiding this comment.
Same comment here as for edit - let's provide fallbacks if data is missing.
There was a problem hiding this comment.
Also the code here could be shared with the edit file.
There was a problem hiding this comment.
I already added a fallback: the fallback is simply the URL.
| /** | ||
| * Default transforms for generic embeds. | ||
| */ | ||
| const transforms = { |
There was a problem hiding this comment.
I tried pasting https://aheadcreative.com into the editor canvas and it turned it into an embed block. I couldn't see how to get the link preview block to work.
There was a problem hiding this comment.
Oh, I've not added that yet. When should we convert a pasted link to a link preview? When it fails to embed?
There was a problem hiding this comment.
If that can be achieved then yes that would seem logical 👍
|
Regarding paste, I'm wondering if we should give people the option between three different blocks when pasting a link:
So upon paste, we could show a placeholder with the three different blocks and a preview of them from which the user can pick. Not sure if this is a good or bad idea. |
Just to be sure: is the Link Preview superior to the current Link Embed block? If it is, it could replace it (or the embed could be "refactored" into this). Or is there a reason to keep the existing link embed block around? That said, one way of transforming one into the other could potentially be this top of the inspector interface that we are discussing for navigation items in #46195. I.e. the toggles. Maybe? |
|
@jasmussen What is the Link Embed? I've never heard of it and I'm also not seeing it in the block library. |
ellatrix
force-pushed
the
try/link-preview
branch
from
17178a9
to
cfa35f5
Compare
|
Not sure how to handle paste. When an embed block is created, there's no way to know it came from paste, and we don't just want to replace any failing embed block to a link preview (for example when you load a post in the editor). We only want it on paste. The paste handler also doesn't know when a block "failed". It seems that the only option right now is to offer it next to "Convert to link" in the placeholder you see when an Embed failed to preview. In any case, I don't think this should be a blocker for adding this new block? Would love some help with the design of this preview. |
There was a problem hiding this comment.
The single fetch in the effect is much better 👍
Tested with a super slow network and noticed that the form remains visible even during fetching. This means that it's possible to update the URL after the initial fetch has been initiated. This leads to potential network race conditions.
We should probably disable the UI whilst fetching - what do you think?
Screen.Capture.on.2023-02-23.at.11-00-39.mp4
I also pushed a change to improve the error notice styling:
Not a blocker. |
Sorry, just coming back to this now. What I mean is, currently when you paste a link on a new line and press enter, it's transformed into a sort of Oembed block that does show a little preview. That's what I called the "Link Embed", but I'm not actually sure what technically it's called — perhaps just the regular Embed block? The main point I wanted to make was that this feature already appears to exist, even if it's hit and miss, and that the superior version should replace whatever is in place at the moment. Does that make sense? |
|
@jasmussen That sort of what we talked about before. Whether this should serve as a fallback if embedding doesn't work. It's two different things though. The current embed block is used for video embedding etc., but not every link can be embedded. This is different: it fetches the title and featured image of a page so you can "preview" the link. I think it's worth starting by adding this block and then figuring out when a pasted link should be embedded or previewed. |
|
I'm mainly wondering about the flow. If I paste a link in Facebook, Twitter, Slack, or otherwise, and give it its own line, I expect a visual preview/embed. If we build a new block that provides a better preview than what occasionally works here, that's cool, but if I have to manually pick that particular block first in order to accomplish that, I worry no-one will discover it, due to the expected embedding flow. |
|
@ellatrix, I've got a block plugin that does the same. Feel free to use any part of it - https://github.com/Mamaduka/bookmark-card.
I don't think that the fetch should live in effect. The block only wants to retrieve data on a particular interaction: when users press the button. I think it would be more suitable to move that logic inside submit event handler. |
|
@ellatrix any update on this block? looks promising. thanks for the great work! |
@Mamaduka Just for context on why this was implemented. Is there a particular reason why you dislike the |
|
Thanks for sharing the context, @getdave! I don't dislike the I think @kevin940726 summarizes the "cons" well in this comment - #48225 (comment).
We can also display the placeholder component with a URL and let the users "embed" the links, instead of auto-embedding them. |
I fully agree with @Mamaduka here. The If it's the only way that could be done to support transforming from and to an embed block, then at least we should document it with a comment so that people don't blindly copy-paste it. |
| } else { | ||
| setHasError( false ); | ||
| setIsEditingUrl( false ); | ||
| setAttributes( { |
There was a problem hiding this comment.
I think we also need a couple of things if this effect is required.
- Add
__unstableMarkNextChangeAsNotPersistentbeforesetAttributesto prevent "undo traps". - Add a cleanup function to prevent race conditions and memory leaks. Don't update states if the component is already unmounted.
|
Just remembered this PR. Is there still interest in pursuing this block? I think we could get it to land by descoping this PR a little even if that compromises initial utility. For example by just shipping the block code and not including the transforms and auto-embedding behaviour. That could all be handled in follow ups. Looks like we need to address a few nits as well. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNCore Committers: Use this line as a base for the props when committing in SVN: GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@getdave I thought it's already pretty descoped. What were you thinking of removing? I would still love this block. :) |
@ellatrix I was thinking this was more complex than it actually is. So nothing to descope. Just fixing up the effectful code. Probably one for post 6.5. |
What?
Adds a new block for previewing links. This could be done as a fallback for embeds, but I think this deserves to be its own block and I don't want to add complexity to the embed block and endpoints (my first attempt at this involved adding it to the embed API and block). We could add a button to convert to a link preview block if embedding fails.
This still needs some design work, and for some reason the stylesheet is not loading on the front end.
Why?
I've been missing this feature. I'd love it if we could automatically create this block when pasting a link on an empty line, but we need to see how this works with the embed block.
How?
Uses the existing
url-detailsendpoint. Is might be that we need to improve this, I don't know how good it is. Currently it's only used to preview links in the link editing UI.Testing Instructions
Create a link preview block and submit any URL.
Testing Instructions for Keyboard
Screenshots or screencast