Skip to content
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

Fix: Embed Block: Match HTML in the editor and frontend #65478

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

hbhalodia
Copy link
Contributor

What?

Why?

How?

  • PR removes the block wrapper div, instead adds figure as a wrapper, so it matches with frontend markup.

Testing Instructions

  1. Open any post/page.
  2. Add embed block.
  3. Add URL to be embeded, check for frontend and backend markup.
  4. It should be similar.

Testing Instructions for Keyboard

  • None.

Screenshots or screencast

Backend

<figure tabindex="0" class="block-editor-block-list__block wp-block is-selected wp-embed-aspect-16-9 wp-has-aspect-ratio wp-block-embed" id="block-8d19fac7-16f5-44ac-8ae0-6630fd12dc9d" role="document" aria-label="Block: YouTube Embed" data-block="8d19fac7-16f5-44ac-8ae0-6630fd12dc9d" data-type="core/embed" data-title="YouTube Embed">
    <div class="wp-block-embed__wrapper">
        <iframe title="Embedded content from www.youtube.com" class="components-sandbox" sandbox="allow-scripts allow-same-origin allow-presentation" width="462" height="260">
        </iframe>
    </div>
</figure>

Frontend

<figure class="wp-block-embed is-type-video is-provider-youtube wp-block-embed-youtube wp-embed-aspect-16-9 wp-has-aspect-ratio">
    <div class="wp-block-embed__wrapper">
        <iframe title="Alexandre Corbeil on Gaining New Skills at Trebas Institute Toronto" width="500" height="281" src="https://www.youtube.com/embed/BOM34PANOHM?feature=oembed" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen="">
    </iframe>
    </div>
</figure>

Copy link

github-actions bot commented Sep 19, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: t-hamano <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. [Block] Embed Affects the Embed Block labels Sep 22, 2024
@t-hamano t-hamano self-requested a review September 22, 2024 13:48
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

There are two things that need to be addressed in order for this PR to move forward.

Fix E2E tests

The figure tag applies to the block itself, not inside it. Therefore, currenEmbedBlock.locator('figure') can no longer find the figure element.

The following changes are required:

diff --git a/test/e2e/specs/editor/various/embedding.spec.js b/test/e2e/specs/editor/various/embedding.spec.js
index fb9746dce5..fe488f9130 100644
--- a/test/e2e/specs/editor/various/embedding.spec.js
+++ b/test/e2e/specs/editor/various/embedding.spec.js
@@ -134,15 +134,15 @@ test.describe( 'Embedding content', () => {
                        'https://developer.wordpress.org/block-editor/reference-guides/block-api/block-attributes/'
                );
                await expect(
-                       currenEmbedBlock.locator( 'figure' ),
+                       currenEmbedBlock,
                        'WordPress valid content. Should render valid figure element.'
-               ).toHaveClass( 'wp-block-embed' );
+               ).toHaveClass( /wp-block-embed/ );
 
                await embedUtils.insertEmbed(
                        'https://www.youtube.com/watch?v=lXMskKTw3Bc'
                );
                await expect(
-                       currenEmbedBlock.locator( 'figure' ),
+                       currenEmbedBlock,
                        'Video content. Should render valid figure element, and include the aspect ratio class.'
                ).toHaveClass( /wp-embed-aspect-16-9/ );

Restore is-type-video CSS class

On the frontend, the figure tag may have classes like is-type-video applied, but not in the editor. We need to restore the removed CSS to the block wrapper.

@hbhalodia
Copy link
Contributor Author

Thank You, @t-hamano for review. I would address the requested feedbacks.

@hbhalodia
Copy link
Contributor Author

Hi @t-hamano, On adding the className to the block wrapper, some functionalities are breaking in the editor, for example, If I do not add the class, then if you click on the video, you can see the block controls, and you can update it, but on adding the class, block controls are not visible, and you would not able to update anything. Here is the screencast for the same.

issue-embed.1.mov

@t-hamano
Copy link
Contributor

We cannot add the className prop directly to the figure element.

The blockProps itself contains classNames, but if we add the className prop after blockProps, the className from blockProps will be overwritten and the block will not work properly.

We will probably need to add the class name via useBlockProps, like so:

const blockProps = useBlockProps( {
	className: clsx( {
		'is-type-video': 'video' === type,
	} ),
} );

@hbhalodia hbhalodia requested a review from t-hamano September 24, 2024 04:38
@hbhalodia
Copy link
Contributor Author

Hi @t-hamano, I have added the requested changes, and added the fix by changing the blockprops.className, this is because, className and type are added after the blockProps is being added, hence we cannot directly add there, so need to update the className in a way.

Thank You,

@t-hamano
Copy link
Contributor

@hbhalodia Thanks for the update!

I've tried several providers, or updating additional CSS classes, etc. and everything seems to work fine.

  • Could you rebase this PR? It should fix the E2E test failure.
  • This might be a matter of preference, but class names might be better for an ad-hoc approach:
    diff --git a/packages/block-library/src/embed/edit.js b/packages/block-library/src/embed/edit.js
    index b69fc75cef..17f6261751 100644
    --- a/packages/block-library/src/embed/edit.js
    +++ b/packages/block-library/src/embed/edit.js
    @@ -251,9 +251,6 @@ const EmbedEdit = ( props ) => {
                    className: classFromPreview,
            } = getMergedAttributes();
            const className = clsx( classFromPreview, props.className );
    -       blockProps.className = clsx( blockProps.className, className, {
    -               'is-type-video': 'video' === type,
    -       } );
     
            return (
                    <>
    @@ -265,7 +262,12 @@ const EmbedEdit = ( props ) => {
                                    toggleResponsive={ toggleResponsive }
                                    switchBackToURLInput={ () => setIsEditingURL( true ) }
                            />
    -                       <figure { ...blockProps }>
    +                       <figure
    +                               { ...blockProps }
    +                               className={ clsx( blockProps.className, className, {
    +                                       'is-type-video': 'video' === type,
    +                               } ) }
    +                       >
                                    <EmbedPreview
                                            preview={ preview }
                                            previewable={ previewable }
    Similar approaches can be found here and here.
  • While testing this PR, I noticed that there were some CSS classes missing from the editor. It would be nice to address this in a follow-up:
    • wp-block-embed-{variationName} (example: wp-block-embed-youtube)
    • is-provider-{providerName} (example: is-provider-youtube)
    • is-type-rich (For example, when you embed Twitter, this CSS class is added to the front-end:)

@hbhalodia
Copy link
Contributor Author

Hi @t-hamano, thanks for the feedback. I would update the PR with requested changes.

@hbhalodia
Copy link
Contributor Author

Hi @t-hamano, I have updated the PR with the requested changes. Can you please review the same.
Thank You,

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the update 👍

@t-hamano t-hamano merged commit d6a4c72 into WordPress:trunk Oct 8, 2024
62 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.5 milestone Oct 8, 2024
@hbhalodia hbhalodia linked an issue Oct 15, 2024 that may be closed by this pull request
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
)

* Add the fix for embed to resemeble exact markup as frontend and backend

* Fix e2e test for updated embed block

* Add the blockprops className to use the newClassname

* Fix the code syntax for the same

* Add the missing class on editor for the embed block

* Add is-type-<typename> class to the editor embed block

Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed Block: Match HTML in the editor and frontend
2 participants