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

Try using the @wordpress/embed-block library #14648

Closed
wants to merge 1 commit into from

Conversation

pento
Copy link
Contributor

@pento pento commented Feb 12, 2020

WordPress/gutenberg#19113 is an experiment in extracting the embed block boilerplate code into a new @wordpress/embed-block library.

This PR is an experiment in making use of that library.

Changes proposed in this Pull Request:

Replace embed block boilerplate code with @wordpress/embed-block.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This improves and standardises existing embed blocks.

Testing instructions:

Download and build WordPress/gutenberg#19113.

Create a docker-compose.jetpack.yml file in the directory above your Jetpack directory:

version: '3.3'
services:
  wordpress:
    volumes:
      - /path/to/gutenberg:/var/www/html/wp-content/plugins/gutenberg

Start the Jetpack Docker environment with this command: yarn docker:compose -f ../docker-compose.jetpack.yml up -d

Proposed changelog entry for your changes:

  • N/A

@pento pento added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Pinterest labels Feb 12, 2020
@pento pento requested review from a team February 12, 2020 00:11
@pento pento self-assigned this Feb 12, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello pento! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D38737-code before merging this PR. Thank you!

@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against b4a7f2d

@zinigor
Copy link
Member

zinigor commented Feb 13, 2020

You're probably going to work on this more, but I thought I'd provide some feedback anyway. I have tested the changes using the Pinterest block, or what's left of it, and I gotta say it looks really promising. Several things that I found lacking so far:

Stable Gutenberg version

When updating to the new code, old blocks become uneditable and ask you to convert them to HTML:
Screenshot from 2020-02-13 11-48-36

Newer blocks work fine even without your Gutenberg PR, but as simple embeds:
Screenshot from 2020-02-13 11-48-40

With your PR enabled
The old block becomes invalid, while the new looks just as the old one, yet becomes uneditable.
Screenshot from 2020-02-13 11-59-45

Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

I looks like this approach removes some of the functionality that the Core embed components don't provide.

For example, the pin.it short urls don't work for me when testing this (I assume because the url is no longer run through the resolve-redirect endpoint to get the destination url for the embed).

Given how the embed blocks in Jetpack have evolved so far (Pinterest, Eventbrite, OpenTable, Calendly), in many cases with similar but slightly different functionality specific to each embed, I think we're going to need more flexibility than the getEmbedBlockSettings function provides in order to support existing functionality.

I'm not all that familiar with the Core embed blocks, but one example might be exporting the EmbedPreview component, since we've copied the same overlay technique into Jetpack's blocks.

@pento
Copy link
Contributor Author

pento commented Feb 14, 2020

Thanks for the testing, and feedback!

@zinigor: You're absolutely right, there's still work to be done on transforming old blocks into the new blocks, as well as general usability. Given that the Gutenberg PR is still highly experimental, I didn't want to get too much into building something, when it could all change.

@creativecoder: The Gutenberg PR is very much a first step, it clearly needs the ability to hook into various parts of the block. The solution for each block may end up being different that a straight port, though. For example, pin.it support potentially doesn't need to be done in the block JS, as it was previously. Instead, pinterest_embed_handler() could be modified to handle pin.it URLs.

@creativecoder
Copy link
Contributor

Instead, pinterest_embed_handler() could be modified to handle pin.it URLs.

Thanks for clarifying! That's a good reminder to consider if there are PHP elements of Core embeds that are useful for the future embed blocks in Jetpack.

@pento
Copy link
Contributor Author

pento commented Feb 27, 2020

Given that block variations are currently being explored in Core for providing embed blocks (see the discussion on WordPress/gutenberg#19113), I'm going to close this PR, I'll likely open a new one when there's something in Core to experiment with.

@pento pento closed this Feb 27, 2020
@pento pento deleted the try/embed-block-library branch February 27, 2020 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Pinterest [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants