-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Embeds: Don't transform into specialized embed block variation if it's not registered #24559
Embeds: Don't transform into specialized embed block variation if it's not registered #24559
Conversation
Size Change: +1.79 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
e43217a
to
a3865e4
Compare
b3eb47e
to
86c4add
Compare
894b7d6
to
c583191
Compare
Should be ready for review 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in principle, but I haven't tested. I'll defer to @ntsekouras for review and approval. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good 👍 - I left some small comments though.
Thanks for the feedback! Should be ready for another look 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment to change the hardcoded value wordpress
but when this change happens let's land this! 👍
Thanks @ockham!
@@ -98,8 +104,7 @@ export const createUpgradedEmbedBlock = ( | |||
// WordPress blocks can work on multiple sites, and so don't have patterns, | |||
// so if we're in a WordPress block, assume the user has chosen it for a WordPress URL. | |||
const isCurrentBlockWP = | |||
providerNameSlug === WP_VARIATION.attributes.providerNameSlug || | |||
type === WP_EMBED_TYPE; | |||
providerNameSlug === 'wordpress' || type === WP_EMBED_TYPE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅 )
@@ -114,14 +119,24 @@ export const createUpgradedEmbedBlock = ( | |||
} ); | |||
} | |||
|
|||
const wpVariation = getBlockVariations( DEFAULT_EMBED_BLOCK )?.find( | |||
( { name } ) => name === 'wordpress' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
Description
Users can insert a generic version of the
core/embed
block 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/embed
block 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/embed
is 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: