Conversation
|
Size Change: +1.79 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
fix/disable-wordpress-embeds-transform-if-variation-unavailable
branch
from
e43217a to
a3865e4
Compare
ockham
force-pushed
the
fix/disable-wordpress-embeds-transform-if-variation-unavailable
branch
from
b3eb47e to
86c4add
Compare
ockham
force-pushed
the
fix/disable-wordpress-embeds-transform-if-variation-unavailable
branch
from
894b7d6 to
c583191
Compare
ockham
requested review from
Soean,
ajitbohra,
ellatrix,
talldan and
youknowriad
as code owners
|
Should be ready for review 🙂 |
mcsf
left a comment
mcsf
left a comment
There was a problem hiding this comment.
Looks good in principle, but I haven't tested. I'll defer to @ntsekouras for review and approval. :)
ntsekouras
left a comment
ntsekouras
left a comment
There was a problem hiding this comment.
This looks good 👍 - I left some small comments though.
|
Thanks for the feedback! Should be ready for another look 🙂 |
ntsekouras
left a comment
ntsekouras
left a comment
There was a problem hiding this comment.
I left a comment to change the hardcoded value wordpress but when this change happens let's land this! 👍
Thanks @ockham!
| const isCurrentBlockWP = | ||
| providerNameSlug === WP_VARIATION.attributes.providerNameSlug || | ||
| type === WP_EMBED_TYPE; | ||
| providerNameSlug === 'wordpress' || type === WP_EMBED_TYPE; |
There was a problem hiding this comment.
Change the hardcoded wordpress to WP_PROVIDER or something similar, put it in constants file like WP_EMBED_TYPE and get it from our variations.js file.
There was a problem hiding this comment.
TBH given my other comment plus the fact that these are currently two different fields, using a constant to compare them both to has the potential to make this more confusing IMO. Using the same constant kind of implies that both fields are somehow functionally equivalent, which they aren't. Using a string literal in both cases (which just happens to be identical) carries that notion much less.
(To clarify, my previous suggestion to drop providerNameSlug was solely meant for the variations definition JSON file -- I didn't mean to remove the provider name check altogether for embeds that we're fetching from a given URL -- at least not before I understand it better 😅 )
| } | ||
|
|
||
| const wpVariation = getBlockVariations( DEFAULT_EMBED_BLOCK )?.find( | ||
| ( { name } ) => name === 'wordpress' |
There was a problem hiding this comment.
Some comment as above to retrieve hardcoded value. Since name and providerNameSlug are the same now, choose what fits more. For example if you choose only the providerName, change this find check to use the same property.
There was a problem hiding this comment.
TBH for the sake of future refactoring, I'd rather stick with name here, since it'll be one thing less to think about for future devs who'd like to phase out providerNameSlug. (Semantically, it also makes more sense to say that we're looking for the right block variation by its name, rather than its provider name.)
Thanks @ntsekouras! I replied to both comments, essentially suggesting to keep the |
I agree with that but still the Either way this is just a small comment and that's why I had pre-approved :) |
Not sure I'm totally following 😅 -- we use the |
|
You're right. Got a bit stuck there with that previous line in mind Land this 💯 |
|
Thanks a bunch @ntsekouras! |
ockham
deleted the
fix/disable-wordpress-embeds-transform-if-variation-unavailable
branch
3 participants
Description
Users can insert a generic version of the
core/embedblock from the block inserter, or choose a more specialized block variation (e.g. for Instagram, YouTube, etc).Furthermore, if a user inserts a plain URL, we also trigger our embed block creation behavior: We first create a generic
core/embedblock from that URL, and then analyze if we have a more specialized block variation. If we do, we transform the generic block into that block variation.A site admit might now choose to unregister certain block variations, in order to prevent users from inserting embeds of a certain type (e.g. they might choose to disallow WordPress embeds out of security concerns).
In that case however, the URL insertion behavior will still attempt to insert a embed block variation for that specific embed type, even though that block variation is not available.
We should thus check if a block variation of the
core/embedis registered for the given embed type before attempting to transform the generic embed block into that variation.Related conversations
#24090 (comment)
Automattic/wp-calypso#39935 (comment)
How has this been tested?
Verify that embeds still work.
The specific use case where a block variation isn't registered should be covered by the unit tests, but you can add some code to unregister a variation manually and insert a matching embed to verify that the generic embed block isn't tranformed to that variation.
Types of changes
Bug fix
Checklist: