-
Notifications
You must be signed in to change notification settings - Fork 81
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
Reply Block: Embed referenced post when possible #1100
base: trunk
Are you sure you want to change the base?
Conversation
150cb7f
to
3db22d2
Compare
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 working really well, nice work!
I've tested it with replies to posts on mastodon.social
as well as smaller instances not mentioned in this PR, and non-Mastodon instances (in which case you can get a 401 on the embed request) and it's all displayed nicely.
I had a question about the design.
On Mastodon, loading a reply clearly displays the reply in focus, and the original post above it in a darker color. In WordPress, the original post naturally gets displayed above the reply so it has more emphasis than the reply you're writing.
I wonder if it would be helpful to clearly indicate "in response to" above the embed, or something like that, so it's clearer that the embed is the original post you're responding to?
@obenland can you maybe add some example screenshots to the PR? |
@pfefferle You can check some examples on my own site right now:
|
Thanks a lot @jeherve ! Should we make this configurable? This is a very big block that we add to each post!?! |
@obenland I added some microformats to match the https://indieweb.org/reply-context
@obenland I added some microformats to match the https://indieweb.org/reply-context |
That's a good question. I personally think it makes sense to provide all that context, but I could imagine some folks wanting a more consise display. How about adding a filter so folks could choose between an expanded and a compact view? Later on and if that proves necessary, we could make that a setting in the Reply block itself. |
Thanks for braving the branch in prod @jeherve 🫡🫡🫡 Got to bottom of the problem in 22be389 and decided to scratch one last itch that's been bugging me with the previous behaviour of "show nothing from the block when federating" - that's not how most platforms work, they show a We should be good, with that, now. Let me know how things are on the UX front, cc @Automattic/fediverse as well, I think that this is really finally actually done (ducks) |
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.
There's no way around the custom API endpoint?
* | ||
* @return string|false The embed HTML or false if not found. | ||
*/ | ||
function get_embed_html( $url, $inline_css = true ) { |
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.
With this function it looks like we can remove embed_get()
?
I liked the attempt in embed_get()
to get an oembed response first, should we do that here, too?
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.
Yes we can almost certainly get rid of oembed_get
but I agree that I like its approach of serving a real oEmbed
where we can get one. It's just that the lifecycle is very tricky because we can only use a pre_oembed_get
filter, we can't filter on a not found
attempt except in the way we're doing it by filtering the endpoint itself. I think there's a way to juggle all of this but it's precarious. But I would like to manage this compexity on the dev side in favour of user simplicity.
'url' => array( | ||
'required' => true, | ||
'type' => 'string', | ||
'sanitize_callback' => 'esc_url_raw', |
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.
'sanitize_callback' => 'esc_url_raw', | |
'format' => 'uri', | |
'required' => true, |
With these changes most checks in validate()
should be possible to remove
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.
Excellent. Nice to have a REST API sleuth on the job to compensate for my Windsurf slop ;)
array( | ||
array( | ||
'methods' => WP_REST_Server::READABLE, | ||
'callback' => array( $this, 'validate' ), |
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.
'callback' => array( $this, 'validate' ), | |
'callback' => array( $this, 'get_items' ), |
Let's reuse the format in WP_REST_Controller?
'sanitize_callback' => 'esc_url_raw', | ||
), | ||
), | ||
'permission_callback' => array( $this, 'validate_url_permissions_check' ), |
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.
'permission_callback' => array( $this, 'validate_url_permissions_check' ), | |
'permission_callback' => array( $this, 'get_items_permissions_check' ), |
We absolutely need it to some degree, it's the only way to give a reliable |
…/oembed-reply-block
06e8329
to
35e7a3f
Compare
Demo'd this internally today. Two notes:
|
oEmbed likes to filter out valid embeds for non-registered providers. So we check for that case, and see if we can help with ActivityPub-specific markup.
…/oembed-reply-block
…/oembed-reply-block
I am going to add some more unit tests but I've fixed the bugs from yesterday's demo and this should be good for a hopefully-final round of testing! What has improved:
|
Fixes #1027.
In this first pass the Mastodon embed provider only gets registered when the Reply block gets rendered. I'm open to making it generally available but initially it didn't feel like functionality that this plugin should be responsible for globally.
There's also an action to register additional Fediverse oembed providers beyond Mastodon.social. Not sure if there are others we should or need to consider.
The current implementation supports embedding posts from all supported oembed providers. I wasn't sure whether it should be limited to Mastodon or whether it should be all—please let me know if you have an opinion on that.
Finally, this maintains the anchor, that IndieWeb seems to be looking for.
Proposed changes:
Other information:
Testing instructions:
Testing Reply Block with Mastodon Posts:
Testing with Non-Embeddable URLs:
Testing with Other oEmbed Providers: