Disable overriding links of images inside pattern instances #58660
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
kevin940726
added
[Type] Bug
kevin940726
requested review from
artemiomorales and
michalczaplinski
as code owners
|
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. |
49 tasks
|
Size Change: +54 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Feb 5, 2024
Comment on lines
441
to
446
| const hasParentPattern = useSelect( | ||
| ( select ) => { | ||
| const { getBlockParentsByBlockName } = select( blockEditorStore ); | ||
| return ( | ||
| getBlockParentsByBlockName( clientId, 'core/block' ).length > 0 | ||
| ); | ||
| }, | ||
| [ clientId ] | ||
| ); |
There was a problem hiding this comment.
Let's select this value via the existing selector. There's no need to create an extra block-editor subscription for every image block.
There was a problem hiding this comment.
Sure! But just being curious, is adding a single store subscription expensive? If so, then I think we might better fix that at a lower-level and make that fast enough. For instance, react-redux's useSelector API doesn't have this problem most of the time.
There was a problem hiding this comment.
It depends on the use case. It could be expensive for very active stores (block editor) and commonly used blocks (Paragraph/Heading/Image). See #54819 (comment).
For instance, react-redux's useSelector API doesn't have this problem most of the time.
Can you share any resources where I can read more about it?
There was a problem hiding this comment.
Can you share any resources where I can read more about it?
It's recommended in the official doc to call useSelector several times even though that creates multiple subscriptions.
Since selectors should be pure and subscriptions should be batched, they should run pretty fast and should rarely be the performance bottleneck. If we see significant performance issues then it's likely other parts of the code, or we have bugs in our useSelect implementation. 🤔
talldan
reviewed
Feb 6, 2024
Comment on lines
425
to
434
| lockUrlControls: | ||
| !! urlBinding && | ||
| getBlockBindingsSource( urlBinding?.source ) | ||
| ?.lockAttributesEditing === true, | ||
| ( !! urlBinding && | ||
| getBlockBindingsSource( urlBinding?.source ) | ||
| ?.lockAttributesEditing === true ) || | ||
| // Disable editing the link of the URL if the image is inside a pattern instance. | ||
| // This is a temporary solution until we support overriding the link on the frontend. | ||
| hasParentPattern, |
There was a problem hiding this comment.
I think it should be in a separate property called lockHrefControls, as the url attribute is for image block's src on the <img> tag (which is confusing).
The attribute name for the <a> tag's href is also href (not as confusing).
The block bindings code also seems wrong for this, which is I think what has caused confusion. You can see it's incorrectly used to lock controls for the href (which is not a supported block binding attribute):
gutenberg/packages/block-library/src/image/image.js
Lines 445 to 459 in 77878ea
There was a problem hiding this comment.
This is true. I noticed this as well, but forgot to mention it after the refactor. href and url are different things in the image block 😅.
There was a problem hiding this comment.
Changed in 34df026. Not sure if we need the hrefBinding part though 🤔.
There was a problem hiding this comment.
I don't think so as it's not a supported binding attribute.
But this should fix the issue, though I would appreciate some testing from block binding contributors, as there might have been a reason that it was like that (though I can't see one).
There was a problem hiding this comment.
I hadn't seen this comment, sorry. I left a similar comment here.
We locked the href controls when the URL is bound because it doesn't make sense to link to the attachment page or the media URL. It is not going to work as expected. It's true that maybe we can try to disable only those instead of the whole href controls.
|
Flaky tests detected in 34df026. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7797040772
|
|
this is testing well for me now but will wait until block binding contributors have taken a look before giving it a ✅ |
Comment on lines
431
to
434
| lockHrefControls: | ||
| ( !! hrefBinding && | ||
| getBlockBindingsSource( hrefBinding?.source ) | ||
| ?.lockAttributesEditing === true ) || | ||
| // Disable editing the link of the URL if the image is inside a pattern instance. | ||
| // This is a temporary solution until we support overriding the link on the frontend. | ||
| hasParentPattern, |
There was a problem hiding this comment.
We don't support the href attribute yet, so hrefBinding should never exist. For sources like post meta, we disabled the href controls when the URL is bound because it didn't make sense to link to the image file or the attachment page. Should we just use something like this instead:
lockHrefControls: hasParentPatternAnd keep the URL conditional later.
There was a problem hiding this comment.
Thanks! I updated it in 9cd4a5e. LMK if I understood you correctly?
There was a problem hiding this comment.
Yes, that's what I meant 🙂
SantosGuillamot
approved these changes
Feb 7, 2024
glendaviesnz
force-pushed
the
fix/pattern-overrides-disable-image-links
branch
from
9cd4a5e to
2063647
Compare
|
👋 Hi @kevin940726! Just a friendly heads up that the |
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?
Part of #53705. Disable editing links of images inside pattern instances.
Why?
The link attributes of the image block mostly depend on the
figure > aselector. The HTML Tag Processor API doesn't support the>syntax yet. (c.c. @artemiomorales, @SantosGuillamot)The link can still be edited but won't be saved when inside a pattern instance. The easiest option for now is just to disable the editing control until we support it.
How?
Disable the edit control when the image is inside a parent pattern instance.
Testing Instructions
Screenshots or screencast