Improve the featured image UI when it cannot retrieve the image file and data #66936
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
afercia
added
[Type] Bug
|
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 If 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. |
|
Size Change: +293 B (+0.02%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Nov 13, 2024
Mamaduka
reviewed
Nov 13, 2024
Mamaduka
reviewed
Nov 13, 2024
Comment on lines
+242
to
+241
| { ( isLoading || | ||
| isRequestingFeaturedImageMedia ) && ( | ||
| <Spinner /> | ||
| ) } |
There was a problem hiding this comment.
Do we need to add the isRequestingFeaturedImageMedia condition here? The values selected via hasFinishedResolution rarely work for loading states.
There was a problem hiding this comment.
Actually, this is the bit that makes the Spinner work also when normally selecting an image from the Media library. Previously, the spinner appeared only when dragging an image to the drop zone. The existing isLoading name is a bit misleading. At first I thought it was to check the actual 'loading' of an image but no, it is true only when dragging. A better name would have been isUploading or something like that.
There was a problem hiding this comment.
The block editor only displays spinners when uploading an image; We shouldn't display a snipper when image data is fetched from the server.
P.S. Snippers should always be a last resort; they make sense as uploading indicators but should be avoided as loading indicators whenever possible, IMO>
There was a problem hiding this comment.
The block editor only displays spinners when uploading an image
Hum, I'm not sure that would be a good design principle. Not even sure it's a documented guideline? I see Spinners used for many things e.g. loading Pages, Patterns, etc. also Spinners used in documentation examples for other things like delete buttons etc.
Anyways, without the Spinner, this UI would only show an empty button which isn't a good pattern IMHO. Screenshot:
Mamaduka
reviewed
Nov 13, 2024
afercia
force-pushed
the
fix/featured-image-missing-image-file
branch
from
d027b29
to
3f3ac4f
Compare
|
@Mamaduka thanks for your review.
Rebased. |
|
Thank you, @afercia! I think this point is still valid. For consistency, we shouldn't display a loading state when fetching image data from the server. |
Thank you @Mamaduka. I kindly disagree. Showing an empty button is less than ideal. The UI state needs to be communicated. It is fetching the image data, to me this is no different from fetching Pages or Patterns and similar states, where a Spinner is used to communicate the retrieval of data. |
Mamaduka
reviewed
Nov 14, 2024
| @@ -250,6 +297,12 @@ const applyWithSelect = withSelect( ( select ) => { | |||
| const { getMedia, getPostType } = select( coreStore ); | |||
| const { getCurrentPostId, getEditedPostAttribute } = select( editorStore ); | |||
| const featuredImageId = getEditedPostAttribute( 'featured_media' ); | |||
| const isRequestingFeaturedImageMedia = | |||
| !! featuredImageId && | |||
| ! select( coreStore ).hasFinishedResolution( 'getMedia', [ | |||
There was a problem hiding this comment.
We're already extracting the coreStore selectors above. Let's extract hasFinishedResolution there as well.
Mamaduka
reviewed
Nov 14, 2024
| @@ -230,6 +271,12 @@ function PostFeaturedImage( { | |||
| onRemoveImage(); | |||
| toggleRef.current.focus(); | |||
There was a problem hiding this comment.
This line triggers an error when trying to remove the missing featured image because the toggle button is no longer rendered. We'll need to fix the focus return on removal logic.
Screenshot
|
Okay, I experimented with the proposed snipper a little more, and it doesn't seem out of place. Generally, we should avoid using spinners as loading indicators when possible. Nobody likes seeing dozens of spinners on a page when they open it. @afercia, I found a bug with focus return logic, which is currently a main blocker here. P.S. I've also found an easier way to replicate the bug. WP CLI command: |
|
@Mamaduka thanks for your thorough review. The focus bug should be fixed now. |
|
Flaky tests detected in 1fc531f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11853840142
|
Mamaduka
approved these changes
Nov 15, 2024
There was a problem hiding this comment.
Also, I really wish we didn't have to resort to using
useEffectandsetTimeoutfor focus management, but that's a more general problem with React. It's not a blocker here.
Yeah, I know. I tried to make the toggle alwasy rendered and toggling its visibility in order to avoid a setTimeout. It didn't work anyways.
afercia
force-pushed
the
fix/featured-image-missing-image-file
branch
from
1fc531f
to
830a8dc
Compare
|
Rebased. |
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.
Fixes #66005
What?
The Post featured image UI doesn't take into account the edge case when there is an attachment ID stored in the database but no actual image file and no image meta data. For example, this may happen when importing posts via the WordPress Importer and skipping to import the attachments. It may happen in other cases as well as migrating the database etc.
In these cases, the UI only shows an empty button. See #66005 (comment)
Why?
The UI should always be clearly operable and provide feedback on the current state including errors or missing attachment / data.
How?
isDestructivestyling for the remove button.Testing Instructions
xmlextension:featured image broken.txt
Download and import file attachmentscheckbox.Broken featured image.wp post meta update your_post_id_here _thumbnail_id 999999.Testing Instructions for Keyboard
Screenshots or screencast
This is the UI when the attachment ID exists but no other data exist:
I have no strong opinions on whether the informative message in the paragraph should have some specific styling e.g. like a warning. I'd lean towards keeping it simple. Cc @WordPress/gutenberg-design
This is the UI with an image correctly set. No changes here:
It's worth noting the case covered here is different from another edge case:
In this case, the UI will show a 'broken image' as browsers do by default, with the alternative text / fallback text displayed. Screenshot:
That is the current behavior of this UI. Hovering or focusing will show the Replace and Remove buttons but they don't look that great on a white background. I'd tend to think the styling for this case should be improved but it's out of the scope of this PR.