Pattern overrides: disallow override for image with caption/href #62747
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
|
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: +147 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
ellatrix
added
[Type] Bug
|
For me, not showing the "Enable Overrides" button when an image has a link or a caption feels a bit confusing. Especially because users won't know why in some images the button exists and not in others. Maybe we should add a message explaining it? Anyway, I believe @talldan will have better context than me. |
|
Sure, we can could disable the button and add a warning maybe. |
This was referenced Jun 21, 2024
|
Another possibility could be to "disallow caption for images with overrides" instead of "disallow overrides for images with caption/href". If I am not mistaken, it should be possible doing something like this: In the image save, check {
!RichText.isEmpty(caption) &&
metadata?.bindings?.__default?.source !== "core/pattern-overrides" && (
<RichText.Content
className={__experimentalGetElementClassName("caption")}
tagName="figcaption"
value={caption}
/>
);
}In the image edit, do the same check: {
metadata?.bindings?.__default?.source !== "core/pattern-overrides" && (
<Caption
attributes={attributes}
setAttributes={setAttributes}
isSelected={isSingleSelected}
insertBlocksAfter={insertBlocksAfter}
label={__("Image caption text")}
showToolbarButton={
isSingleSelected &&
(hasNonContentControls || isContentOnlyMode) &&
!hideCaptionControls
}
readOnly={lockCaption}
/>
);
} |
I prefer this option. 👍 |
talldan
changed the title
I'm not sure I understand this. So you'd remove the caption of an image once you press the override button? That seems unexpected too? |
Yes, you are right that seems unexpected too. I was just sharing it to know more thoughts. For me, it's fine to follow this approach if disable the "Enable Overrides" button and add a message asking the user to remove caption and links. I started a draft pull request to explore the other approach, but I must say this one seems safer because it doesn't interfere with the content already created by the users. |
|
With this PR, couldn't the user first enable overrides and then add the caption? I don't think this approach can completely solve the full issue, and so I wonder whether it's worth it. There are probably a lot of small ways to workaround it that lead to the user still encountering problems. FWIW, I'm personally not against keeping how it works in trunk. I think users would pretty quickly discover that captions can't be overridden and not use image captions with overrides. Could also consider a simpler option of adding help text mentioning 'Overrides currently do not support caption text and links in the image block' next to the Enable Overrides button (for the image block only), and then letting the user decide what to do. |
|
I don't think you can? 
|
When creating/editing the pattern you can 'Enable Overrides' then add a caption to the image: So that's one way of achieving what this PR tries to prevent. |
|
Maybe we need something like this?
To me, it is tricky and a bit confusing because it can create weird user experiences. Preventing pattern creators from enabling overrides for images with captions or links seems safer. Personally, I would add two things to this pull request:
With that, I think the user experience would be much better and we would avoid unexpected results. |
|
@SantosGuillamot Ok, lets try that. Shall we push straight to this PR? If I handle the first item you mention, are you ok to push the changes for the second? |
As long as it is fine for @ellatrix , I can help push this forward however it is needed 🙂 |
|
Feel free, of course :) |
|
I've pushed a commit with the change mentioned: |
talldan
added
the
[Feature] Synced Patterns
ellatrix
commented
Jun 24, 2024
| > | ||
| <Button | ||
| __next40pxDefaultSize | ||
| className="pattern-overrides-control__allow-overrides-button" | ||
| variant="secondary" | ||
| aria-haspopup="dialog" | ||
| onClick={ () => { | ||
| if ( hasUnsupportedImageAttributes ) { |
There was a problem hiding this comment.
Is the onClick handler called after disabling it?
There was a problem hiding this comment.
No, looks like the button component will replace the event handler with one that stops clicks. I've pushed another commit to remove this if statement
Oh, that one. Yes, we should disable it when an image block is being overridden. |
SantosGuillamot
requested review from
artemiomorales and
michalczaplinski
as code owners
|
I've pushed another commit to hide the link and caption controls. The caption logic is slightly different from the href controls because when an image without overrides is used in a pattern instance, we still want to show the caption as "read-only". For |
|
It works well for me in testing:
|
talldan
force-pushed
the
try/disallow-image-with-caption-overrides
branch
from
c8e46f4
to
3396ab7
Compare
|
I've rebased the branch to see if it gets the tests passing |
talldan
approved these changes
Jun 24, 2024
SantosGuillamot
approved these changes
Jun 24, 2024
ellatrix
commented
Jun 24, 2024
| /> | ||
| { /* Don't add caption component and controls in images with pattern overrides */ } | ||
| { ! arePatternOverridesEnabled && ( | ||
| <Caption |
There was a problem hiding this comment.
Why do we need this? Can't we just disable the caption button?
There was a problem hiding this comment.
I just assumed we didn't need the Caption component. But I guess we could just disable the button. Maybe changing this property?
…s and disable the Enable Overrides button
ellatrix
force-pushed
the
try/disallow-image-with-caption-overrides
branch
from
3396ab7
to
3105da8
Compare
|
Flaky tests detected in 68af3d9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9646536064
|
29 tasks
|
I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: aae9d92 |
ellatrix
added
Backported to WP Core
This was referenced Jun 25, 2024
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?
Fixes #62287 (the remainder of it, the other part was fixed by #62471)
When creating pattern overrides, disallow creating an override for an image with either a caption or href/link.
Why?
Because it's not supported by block bindings right now.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast