Post feature image block: wrap images with hrefs in an A tag #55498
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
ramonjd
added
[Type] Bug
ramonjd
requested review from
apeatling,
tellthemachines,
alexstine and
andrewserong
ramonjd
commented
Oct 20, 2023
|
Size Change: +297 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Nov 8, 2023
There was a problem hiding this comment.
This is generally working nicely for me, great catch that the default border for a linked image will use the link color!
One thing I noticed in testing is that the placeholder state for the featured image block when rendered within a Query Loop is for it to use a div and no a tag. Is it worth adding the a tag in that case, too (wrapped around the placeholder()? Here's the relevant code:
gutenberg/packages/block-library/src/post-featured-image/edit.js
Lines 211 to 218 in ba56c3b
Without that, there's an inconsistency in the site editor between "real" and placeholder featured image blocks:
Good spotting, thanks! I think it is worth making them consistent. I'll take care of it. |
ramonjd
force-pushed
the
update/post-featured-image-a-tag-when-linked
branch
from
ba56c3b
to
a2591df
Compare
|
Did a quick wrap around the placeholder content (the Seems to work okay so far, I'll test more tomorrow. 2023-11-08.17.24.53.mp4Thanks again! |
…d, and ensures any styles the IMG elements inherits from the A element are also displayed in the editor. The cursor style should be default since, in the editor, we don't want to indicate that the image is clickable.
…onents CSS, which has an immediate child selector to show the button. With the A tag, the placeholder is no longer the immediate child.
ramonjd
force-pushed
the
update/post-featured-image-a-tag-when-linked
branch
from
a2591df
to
6c800b2
Compare
ramonjd
commented
Nov 9, 2023
| // When the Post Featured Image block is linked, | ||
| // and wrapped with a disabled <a /> tag | ||
| // ensure that the placeholder items are visible when selected. | ||
| &.is-selected .components-placeholder.has-illustration { |
There was a problem hiding this comment.
Overwrites placeholder component CSS
|
There was a small bug with the post editor placeholder media upload button not showing, but that's fixed now 😅 |
|
Flaky tests detected in 6c800b2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6807178175
|
andrewserong
approved these changes
Nov 9, 2023
There was a problem hiding this comment.
This is all testing nicely for me now with the placeholder state working correctly in the post editor, site editor, and within a Query Loop 👍
Looks like there are some intermittent test failures across a few PRs today, I think the most common failure I've seen is to do with waiting for the selector for deactivating test plugins:
That all seems very unrelated to this PR.
This is looking good:
LGTM! ✨
cbravobernal
pushed a commit
that referenced
this pull request
Nov 14, 2023
* Wrap post featured image in an A tag so that it simulates the frontend, and ensures any styles the IMG elements inherits from the A element are also displayed in the editor. The cursor style should be default since, in the editor, we don't want to indicate that the image is clickable. * - also wrap the placeholder in an a tag for consistency * Ensure that the placeholder items are visible by overwriting the components CSS, which has an immediate child selector to show the button. With the A tag, the placeholder is no longer the immediate child.
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.
Context: #55336 (comment)
Props to @TheRadicalDreamer
What?
From the back catalogue of wrapping A tag hits, comes: "Let's wrap the image element in an A tag (in the editor only)"!
So, in the editor, where
isLinkistruelet's wrap the featured image in an A tag!Why?
Copy paste quote from #55470:
Also, the CSS change is to force the cursor style to be default since, in the editor, because we don't want to indicate that the image is clickable.
How?
As in #55470, wraps post featured image in an A tag so that it simulates the frontend, and ensures any styles the IMG elements inherits from the A element are also displayed in the editor.
Testing Instructions
For testing I've used Empty Theme, but any block theme will do.
To ensure we can see the effect, it's important to set different colors for the body background and element links. Here are some test styles to whack into your theme.json.
Some example Query block code
Testing Instructions for Keyboard
Screenshots or screencast
2023-10-20.14.26.53.mp4