-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Added class information for core/embed in the frontend #4118
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
* External dependencies | ||
*/ | ||
import { parse } from 'url'; | ||
import { includes } from 'lodash'; | ||
import { includes, kebabCase, toLower } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
|
@@ -11,6 +11,7 @@ import { __, sprintf } from '@wordpress/i18n'; | |
import { Component, renderToString } from '@wordpress/element'; | ||
import { Button, Placeholder, Spinner, SandBox } from '@wordpress/components'; | ||
import { addQueryArgs } from '@wordpress/url'; | ||
import classnames from 'classnames'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -50,6 +51,12 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k | |
align: { | ||
type: 'string', | ||
}, | ||
type: { | ||
type: 'string', | ||
}, | ||
providerNameSlug: { | ||
type: 'string', | ||
}, | ||
}, | ||
|
||
transforms, | ||
|
@@ -70,6 +77,7 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k | |
type: '', | ||
error: false, | ||
fetching: false, | ||
providerName: '', | ||
}; | ||
} | ||
|
||
|
@@ -100,6 +108,7 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k | |
event.preventDefault(); | ||
} | ||
const { url } = this.props.attributes; | ||
const { setAttributes } = this.props; | ||
const apiURL = addQueryArgs( wpApiSettings.root + 'oembed/1.0/proxy', { | ||
url: url, | ||
_wpnonce: wpApiSettings.nonce, | ||
|
@@ -114,11 +123,15 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k | |
return; | ||
} | ||
response.json().then( ( obj ) => { | ||
const { html, type } = obj; | ||
const { html, type, provider_name: providerName } = obj; | ||
const providerNameSlug = kebabCase( toLower( providerName ) ); | ||
|
||
if ( html ) { | ||
this.setState( { html, type } ); | ||
this.setState( { html, type, providerNameSlug } ); | ||
setAttributes( { type, providerNameSlug } ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're setting this into attributes, is there a need on the previous line to set into state as well? Wouldn't this be redundant, and at worst potentially fall out of sync? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I have tested we actually need both. The state delivers the html, while the attributes deliver stuff that is needed on save. But I am far away from understanding the inner workings of react. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noting that because this sets attributes, when loading an existing post which had contained the previous embed block, it will immediately become marked as "dirty" (i.e. needing save) because its attributes have changed. I guess this would be expected though. |
||
} else if ( 'photo' === type ) { | ||
this.setState( { html: this.getPhotoHtml( obj ), type } ); | ||
this.setState( { html: this.getPhotoHtml( obj ), type, providerNameSlug } ); | ||
setAttributes( { type, providerNameSlug } ); | ||
} else { | ||
this.setState( { error: true } ); | ||
} | ||
|
@@ -219,14 +232,20 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k | |
}, | ||
|
||
save( { attributes } ) { | ||
const { url, caption, align } = attributes; | ||
const { url, caption, align, type, providerNameSlug } = attributes; | ||
|
||
if ( ! url ) { | ||
return; | ||
} | ||
|
||
const embedClassName = classnames( 'wp-block-embed', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class should be auto-generated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically yes, but this is only true on the core/embed block. If the user chooses one of the flavored embed blocks, e.g. core/embed-youtube, the class will change to wp-block-embed-youtube and the You could mitigate that in CSS with unqualified attribute selectors (as described in #4107), but that would be at a huge cost for frontend rendering performance. So I am currently thinking about how to avoid class duplication on core/embed, the behavior on the flavored embeds is as intended. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since for a YouTube embed, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: We'd probably expect the fixture markup to be different for most embeds, i.e. including
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 'is-provider-' classes are needed for two reasons:
As for the fixture markup, how would I approach that? Do I have to add that by hand? |
||
[ `is-align${ align }` ]: align, | ||
[ `is-type-${ type }` ]: type, | ||
[ `is-provider-${ providerNameSlug }` ]: providerNameSlug, | ||
} ); | ||
|
||
return ( | ||
<figure className={ align ? `align${ align }` : null }> | ||
<figure className={ embedClassName }> | ||
{ `\n${ url }\n` /* URL needs to be on its own line. */ } | ||
{ caption && caption.length > 0 && <figcaption>{ caption }</figcaption> } | ||
</figure> | ||
|
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.
Does
kebabCase
not handletoLower
already?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.
No, afaik
kebabCase
handleslowerCase
, nottoLower
, which leads to ugly transformations fromYouTube
toyou-tube
.