Embed block: Fix inline preview cut-off when editing URL #35326
Merged
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
fluiddot
added
[Type] Bug
|
Size Change: +45 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
Looks like some of the e2e tests defined in Expected: Received: I'll investigate this issue further and provide a solution as soon as possible 💨 . |
7 tasks
I've addressed this issue in this commit 🔧 . |
jd-alexander
reviewed
Oct 7, 2021
jd-alexander
reviewed
Oct 7, 2021
jd-alexander
approved these changes
Oct 15, 2021
There was a problem hiding this comment.
Thanks for these changes @fluiddot Tested and verified that the cut-off issue is no longer present. LGTM 🚢
|
Hi folks, Is this PR good to merge? Maybe rebase just to be certain tests are still passing. |
fluiddot
force-pushed
the
fix/embed-block-preview-cut-off
branch
from
057e7bb to
de7db7d
Compare
I've just rebased with Before considering merging the PR, I'd appreciate having another review since this change affects the web version. @Mamaduka I'm wondering if you would be able to take a look, thanks for the help 🙇. |
SiobhyB
pushed a commit
that referenced
this pull request
Apr 26, 2022
* Use empty classname when handling incoming preview * Force sandbox render when type prop changes * Add ignore previous classname option to merged attributes * Move get merged attributes function to util class
4 tasks
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.
Description
This PR changes the calculation of attributes when handling an incoming preview after editing the URL of an embed block. Previously, the attributes were obtained from the
getMergedAttributesfunction, however, this function could return the classname from the previous embed, which depending on the new embed could lead to rendering issues, as the cut-off mentioned in wordpress-mobile/gutenberg-mobile#4059.These rendering issues are caused when the previous embed adds the aspect ratio classes to the classname attribute, then after setting a different URL, the new embed expects to have that attribute empty. In the following code references, you can see the flow of how the classname is calculated when handling an incoming preview:
gutenberg/packages/block-library/src/embed/edit.native.js
Line 184 in c357fc7
gutenberg/packages/block-library/src/embed/edit.native.js
Lines 137 to 143 in c357fc7
gutenberg/packages/block-library/src/embed/util.js
Lines 243 to 260 in c357fc7
gutenberg/packages/block-library/src/embed/util.js
Lines 284 to 288 in c357fc7
gutenberg/packages/block-library/src/embed/util.js
Lines 177 to 227 in c357fc7
The calculation is now done in a similar way as the
getMergedAttributesfunction but passing an empty classname ingetAttributesFromPreview. This way we assure that the classname attribute set to the block doesn't preserve the value from the previous embed.This change also required updating the side-effects of the
Sandboxcomponent that updates the HTML of the iframe. When thetypeprop is modified (this prop is used to pass the classname), we need to force rendering the HTML of the iframe so it renders the proper aspect ratio styles.How has this been tested?
Screenshots
embed-block-cutoff-issue-before.mp4
embed-block-cutoff-issue-after.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).