Refactor embed block to single block with block variations #24090
Conversation
ntsekouras
added
[Type] Task
|
Size Change: +72 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
One thing we can't forget is to make sure the old types are still supported, i.e. One way is to add support at the parser level, as was done for Social Icons: gutenberg/packages/blocks/src/api/parser.js Lines 432 to 439 in 201ca82 There's also this comment from Greg about a different way of managing this, but we don't need to solve that now: https://github.com/WordPress/gutenberg/pull/19887/files#r375181162 |
|
I pushed 6d3f5fe to scratch a personal itch: some of these effects become a lot clearer (and more autonomous?) by keeping their specific logic in one place. |
ntsekouras
force-pushed
the
refactor/embed-block-variations
branch
2 times, most recently
from
70c3142
to
a5abd79
Compare
ntsekouras
force-pushed
the
refactor/embed-block-variations
branch
from
c627a4d
to
83c1738
Compare
ntsekouras
requested review from
ajitbohra,
ellatrix,
Soean,
talldan and
youknowriad
as code owners
ntsekouras
force-pushed
the
refactor/embed-block-variations
branch
from
bacbf07
to
66854bf
Compare
ntsekouras
force-pushed
the
refactor/embed-block-variations
branch
from
6399f50
to
65111df
Compare
|
I noticed something when testing this with the Gutenberg demo (
Observed in
Observed in this branch: the embed looks fine before switching modes, but not after:
|
|
On the native mobile side, we do not support any embed blocks yet, but we did see an issue after this change related to how we direct users to edit unsupported (native mobile) blocks from the mobile editor. A GIF demonstrating that flow can be seen on this PR. We left a note with mobile devs who worked on that flow and should have more info soon on necessary mobile side changes. |
Thanks for the ping @ntsekouras and for looking into this @cameronvoell ! If possible Nik, can we momentarily hold merging this PR until we get a better idea on how to fix/avoid the mobile breakage (@etoledom is looking deeper into this)? Let me know if there is any kind of urgency for landing this PR though, thanks! |
|
It seems to me that (Please take with a grainof salt, this is the result of some preliminary testing; I'll continue to look into this tomorrow.) |
|
ping @ntsekouras about my #24090 (comment):
Can you have a look what might've changed there? 🙏 Furthermore, related to this comment: Automattic/wp-calypso#39935 (comment):
I'm having trouble understanding the point of the Furthermore, does it work like it's supposed to? For example, if I embed any recent posts from ma.tt, the |
In time of development I had observed a weird behaviour of We came to a conclusion not to alter any related functionality as that PR's main goal was the refactoring to a single block and not take a deep dive into
For now it's the same but I don't know if it would be best to remove it. It feels like a |
|
Thanks for getting back to me!
The point is that it always seems to be
I'm a bit confused since I thought you said that the matching algorithm was changed to respect Overall, I agree on the approach of not making functional changes in a refactor PR -- I'm really just trying to find out what might've inadvertently broken the WordPress embed transformation.
That makes sense for data that we fetch from 3rd party resources (like those embeds), but not so much for a hardcoded JSON, does it? 🤔
This seems like a textbook case of the YAGNI pattern -- we might need something in the future, but for now, the |
I can't because I had tried this when working on this PR and wasn't able to get
There was no change regarding the handling of
Yes it does and it overrides the property |
|
Question here: What happens to existing blocks? Do we keep them as hidden or something? Do we want to write a dev note about this change? |
Backwards compatibility is handled in parser level in a similar fashion with gutenberg/packages/blocks/src/api/parser.js Line 441 in 6251366
A dev note about informing the change of |
|
@ntsekouras yes, what happens if a user is using the old blocks in a CPT template for instance where the parser is not used? |
|
It would be great to expose Line 16 in f724402 We should also expose it in WordPress core, it needs to be added at least here: |
|
Good catch. |
See Automattic/wp-calypso#55357 Before WP 5.6 and this PR: WordPress/gutenberg#24090 WordPress had multiple embed blocks, one for each service, all starting with core-embed/. After WP 5.6, we now have a single embed block, core/embed, with variations for each service. Let's support both formats in the Responsive Videos.
Resolves: #22660
Above issue is part of #16209 that is a direction toward server-side awareness of block types.
This PR refactors
embedblock to be a single block ( singleblock.json) withblock variations, so as to register in on the server to be exposed in the REST API endpoint with block definitions.Checklist: