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

Extract Embed Block template code to a package. #19113

Closed
wants to merge 9 commits into from

Conversation

pento
Copy link
Member

@pento pento commented Dec 13, 2019

Description

Fixes #13490.

This PR extracts the common code used by Core's embed blocks into a new @wordpress/embed-block package, allowing plugins to make use of this simpler method for creating embed-style blocks.

How has this been tested?

Automattic/jetpack#14648 is an experiment in making use of this new package.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@pento pento added [Type] Enhancement A suggestion for improvement. [Feature] Extensibility The ability to extend blocks or the editing experience [Block] Embed Affects the Embed Block labels Dec 13, 2019
@pento pento self-assigned this Dec 13, 2019
@pento pento added the Needs Technical Feedback Needs testing from a developer perspective. label Dec 13, 2019
@pento pento requested review from youknowriad and gziolo December 13, 2019 02:59
@pento
Copy link
Member Author

pento commented Dec 13, 2019

This is currently a first pass, but I'd appreciate some early feedback on the direction. 🙂

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This is a very interesting idea to move all common code related to embeds to its own package. In fact, there is some prior-art for media utils. See @wordpress/media-utils package introduced by @jorgefilipecosta. It's definitely a good direction if we want to make it easier to build custom embeds.

The biggest challenge is how to ensure that only essential APIs are exposed from this package and are well documented. In particular utils.js files, it seems like it shouldn't be part of the public API.

@pento
Copy link
Member Author

pento commented Dec 13, 2019

The biggest challenge is how to ensure that only essential APIs are exposed from this package and are well documented. In particular utils.js files, it seems like it shouldn't be part of the public API.

I agree, this will need auditing. I just went with export * while hacking it together to get it working. 🙂

@pento pento force-pushed the try/moving-embed-block-template-to-package branch from b56162c to d8d21dd Compare February 11, 2020 22:35
@pento
Copy link
Member Author

pento commented Feb 12, 2020

I have some time to push this forward again, so I've updated the PR, and created a sample use: Automattic/jetpack#14648

@gziolo Do you have more thoughts on the viability of this approach?

@gziolo
Copy link
Member

gziolo commented Feb 17, 2020

@mcsf, did you have a chance to look at embeds and how they could possibly be refactored to sue variations? Still, it might be necessary to expose some components as proposed in this PR.

@mcsf
Copy link
Contributor

mcsf commented Feb 25, 2020

@mcsf, did you have a chance to look at embeds and how they could possibly be refactored to sue variations? Still, it might be necessary to expose some components as proposed in this PR.

(Oops, I missed this ping due to some AFK time.)

I looked at embeds very experimentally as a prelude to the work on Social Icons, which was more pressing. I still think it's fairly feasible to turn Embeds into variations of a single block type. @pento, if you're interested in this, I can cc you in that task so we can figure out what makes most sense to extract from block-library.

@mcsf
Copy link
Contributor

mcsf commented Feb 25, 2020

@gziolo: looking at embeds, it becomes clear that we need slash-inserter support for variations.

@gziolo
Copy link
Member

gziolo commented Feb 25, 2020

@gziolo: looking at embeds, it becomes clear that we need slash-inserter support for variations.

I'm putting it on my todo list 👍

@pento
Copy link
Member Author

pento commented Feb 26, 2020

@mcsf: I don't know that I'll have wild amounts of time to devote to this, but if you can give me some more details on what needs to be done, I'll see what I can wrangle.

@mcsf
Copy link
Contributor

mcsf commented Feb 26, 2020

@mcsf: I don't know that I'll have wild amounts of time to devote to this, but if you can give me some more details on what needs to be done, I'll see what I can wrangle.

@pento: don't worry, I'll share a PR whenever I have something and it should become apparent what could be reused. Honestly, though, my wish is that we could, over the next quarter, greatly simplify the way that embeds work (and turning them into a variation would be a definite first step) so that there is actually very little that needs to be reused. I suspect that advances in React from the last year and half (cf. hooks) could help us untangle some design patterns in the embeds code base, too.

@pento
Copy link
Member Author

pento commented Feb 27, 2020

Sounds like a plan, @mcsf. I'm going to close this PR out, then, and we'll pick it back up again when you've had a chance to explore the block variation idea.

@pento pento closed this Feb 27, 2020
@pento pento deleted the try/moving-embed-block-template-to-package branch February 27, 2020 00:20
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 [Feature] Extensibility The ability to extend blocks or the editing experience Needs Technical Feedback Needs testing from a developer perspective. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose embed block creation function
3 participants