-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show preview of WordPress embed in editor #25370
Conversation
Size Change: -31.7 kB (2%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this!
Can you conceive a test for this?
It's not complete. The problem here is that the responsiveness of the In the editor is not and now it has bigger height, leaving a gap between the iframe and the caption. Aside the incorrect view, if you click the gap and throws an error. I have made some experiments, one of which and IMO the best one, is to attach a Any ideas for this are welcome. :) --Update |
Actually all embedding tests are mocked (their response). One reason for this should be not to have to handle the case this PR handles, to attach listeners etc for iframe content loading.. @mcsf |
const doc = new DOMParser().parseFromString( html, 'text/html' ); | ||
doc.querySelector( 'iframe' ).removeAttribute( 'style' ); | ||
doc.querySelector( 'blockquote' ).style.display = 'none'; | ||
if ( this.node.current ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of life cycle, is this the right place to do this?
What about using callback refs like:
function processContents( ref ) {
if ( ref ) {
ref.querySelector( ... ).style.display = 'none';
...
}
<div ref={ processContents }>...</div>
* | ||
* @param {WPSyntheticEvent} event Message event. | ||
*/ | ||
resizeWPembeds( { data: { secret, message, value } = {} } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all reads a bit like dark magic. :) I had never come across this height signalling before.
I'm not too confident to approve the change.
In any case, providing some context and linking to adequate prior art would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! I've updated the jsdoc.
I test this locally and it is showing a nice progress! |
73dc3d9
to
0592fea
Compare
You're right here, it needs fixing. I'll look into it |
Hey @ntsekouras As if the embed was an image block. With http://localhost:8888/ |
@paaljoachim I fixed the edit problem now. Regarding your screenshot, I don't see anything weird there.. A caption should exist there, if that is what concerns you. |
It sounds like you are saying that the caption should be there for a WP embed. In that case what I thought was a bug is just a feature. Then it is working fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this. :)
Description
Fixes: #25344
Fixes: #21945
Fixes: #21029
Great description in this issue about the root of the problem in this issue: #25344
How has this been tested?
Locally
Checklist: