LinkControl: Add support for images in attachment search results #58458
+325
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MaggieCabrera
commented
Jan 30, 2024
| icon = ( | ||
| <img | ||
| className="block-editor-link-control__search-item-media-thumbnail" | ||
| src={ attachmentImg } |
There was a problem hiding this comment.
this loads the original sized image, it feels a little overkill
There was a problem hiding this comment.
Both this and a thumbnail for the image are on coreStore, but the component shouldn't have to know about core-data. Should we pass that extra info to all instances of the link preview, or should we leave this as is?
There was a problem hiding this comment.
How big is the image? If it's huge then I don't think this is acceptable.
I did have a plan to use the value.id to fetch the full data about the entity from the Core Store. In fact I did have a PR about precisely that but I cannot locate it.
This selector would need to be passed in via blockEditorSettings because - as you noted - we cannot consume Core Store directly here.
If we do this I would consider whether it's better to do that bit as a separate PR.
There was a problem hiding this comment.
#50998 this one perhaps?
|
Size Change: +17.4 kB (+1%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f8eb929. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7827533259
|
MaggieCabrera
added
the
[Feature] Link Editing
getdave
reviewed
Feb 1, 2024
Comment on lines
6
to
10
| export default function isImageUrl( url ) { | ||
| const pattern = /(http(s?):)([/|.|\w|\s|-])*\.(?:jpg|gif|png)/g; | ||
| return pattern.test( url ); | ||
| } |
There was a problem hiding this comment.
Some simple tests for this would be great.
There was a problem hiding this comment.
I just saw that there is another PR (#57824) open for the same thing and I prefer how they implement this check using mimetypes instead of a regex. I was going to grab that implementation myself
There was a problem hiding this comment.
I spoke too soon! seems like it can be done for the suggestions because they come from fetchLinkSuggestions but the link previews don't have that, so we'd still have to figure out if the URL is an image via proding at the actual URL. I'll go about doing some tests for this function then.
| if ( richData?.icon ) { | ||
| icon = <img src={ richData?.icon } alt="" />; | ||
| } else if ( attachmentImg ) { | ||
| //TODO we don't have an alt text for this image |
There was a problem hiding this comment.
An empty alt tag is acceptable when there is no way to provide an accurate description
| icon = ( | ||
| <img | ||
| className="block-editor-link-control__search-item-media-thumbnail" | ||
| src={ attachmentImg } |
There was a problem hiding this comment.
How big is the image? If it's huge then I don't think this is acceptable.
I did have a plan to use the value.id to fetch the full data about the entity from the Core Store. In fact I did have a PR about precisely that but I cannot locate it.
This selector would need to be passed in via blockEditorSettings because - as you noted - we cannot consume Core Store directly here.
If we do this I would consider whether it's better to do that bit as a separate PR.
|
@getdave the image is as big as the original image is. In my local env I was using huge images for some other reason and I was seeing a 2k pixel width image in the tiny thumbnail. It could definitely be a followup |
If I've understood you correctly, I don't think we should introduce the possibility of loading giant images in a preview for a link. |
I might have not been clear, this is what I can see in my test site 
|
|
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. |
|
This PR is cool in terms of improving the UI of adding links. i think for sighted users the small area of preview can speed up choosing the image picking process of choosing from the suggestions or search results. However, we need to show a really tiny thumbnail there not the full image. Can't we get the thumbnail via another REST call and replace async the previews with what we receive from the server? Also for unsighted users the Alt can say "Thumbnail preview of [filename]". |
|
As @draganescu indicated, we can alter fetchLinkSuggestions to query the Media endpoint rather than search. We would need to be sure to continue to offer the same properties on the returned object but we might get access to more data about the media which we could use to augment the response. This PR is already going down that route for the purposes of localization. Worth syncing up there by reviewing that PR and then discussing collaboration. |
Co-authored-by: petitphp <petitphp@git.wordpress.org>
MaggieCabrera
force-pushed
the
add-image-attachment-previews
branch
from
363109f
to
7f1a659
Compare
MaggieCabrera
commented
Feb 7, 2024
Comment on lines
+2
to
+6
| /** | ||
| * REST API: WP_REST_Media_Search_Handler class | ||
| * | ||
| * @package WordPress | ||
| * @subpackage REST_API |
There was a problem hiding this comment.
I replicated the intention of this PR and gave credit to the author in case this one gets merged first
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.6/class-wp-rest-media-search-handler-gutenberg-6-6.php ❔ lib/compat/wordpress-6.6/rest-api.php ❔ lib/load.php |
|
I added the new endpoint and added a thumbnail and alt text to it. Should we remove the link preview implementation from this PR and do that in a follow-up to keep this one simpler? |
draganescu
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Closes #57776
Adds thumbnails for attachments that are images on link preview and the link control search list
Why?
An image gives you more info than the current icon for all attachments
How?
By checking if the URL from the attachment is an image or not
Screenshots or screencast
Attachments under the search result:
Link preview of attachment that is NOT an image
Link preview of attachment that is an image
