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.

Sign up for GitHub

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

Embed Block: Border support added #66386

Open
wants to merge 13 commits into
base: trunk
Choose a base branch
from
+51 −6
Open

Embed Block: Border support added #66386

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1398e28
Border support added for Embed block
benazeerhassan1909 Oct 23, 2024
99b4f17
Border support added for Embed block
benazeerhassan1909 Oct 23, 2024
519f7b7
Merge branch 'add/embed-border' of https://github.com/benazeer-ben/gu…
benazeerhassan1909 Oct 24, 2024
4184b4b
Revert "Style Book: Add color tab (#65692)"
benazeer-ben Oct 25, 2024
03a5108
Merge branch 'trunk' into add/embed-border
benazeer-ben Oct 25, 2024
920cf53
Synced with trunk and resolved conflicts
akasunil Oct 28, 2024
a2e68b4
Revert unneccessory changes
akasunil Oct 28, 2024
e267978
Feedback changes - Skip serialization & border box
benazeer-ben Nov 15, 2024
38ca863
Skip serialization for padding
benazeer-ben Nov 25, 2024
cc18957
Doc Build
benazeer-ben Nov 26, 2024
b0e2645
Update for wordpress embed case border styling
benazeer-ben Nov 26, 2024
3be64f4
Update packages/block-library/src/embed/block.json
benazeer-ben Nov 27, 2024
cc10820
Feedback changes - added wrapperProps
benazeer-ben Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/block-library/src/embed/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,24 @@
},
"interactivity": {
"clientNavigation": true
},
"__experimentalBorder": {
"radius": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

The border support is opt-in by default so we shouldn't have to explicitly set it to false. That said, this does communicate that it was a conscious decision to omit border-radius support so we can leave it..

"color": true,
"width": true,
"style": true,
"__experimentalDefaultControls": {
"radius": false,
benazeer-ben marked this conversation as resolved.
Show resolved Hide resolved
"color": true,
"width": true,
"style": true
},
"__experimentalSkipSerialization": true
}
},
"selectors": {
"border": ".wp-block-embed__wrapper"
},
"editorStyle": "wp-block-embed-editor",
"style": "wp-block-embed"
}
21 changes: 18 additions & 3 deletions packages/block-library/src/embed/embed-preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import clsx from 'clsx';
*/
import { __, sprintf } from '@wordpress/i18n';
import { Placeholder, SandBox } from '@wordpress/components';
import { BlockIcon } from '@wordpress/block-editor';
import {
BlockIcon,
__experimentalUseBorderProps as useBorderProps,
} from '@wordpress/block-editor';
import { useState } from '@wordpress/element';
import { getAuthority } from '@wordpress/url';

Expand All @@ -31,6 +34,7 @@ export default function EmbedPreview( {
className,
icon,
label,
attributes,
} ) {
const [ interactive, setInteractive ] = useState( false );

Expand Down Expand Up @@ -64,16 +68,27 @@ export default function EmbedPreview( {
className,
'wp-block-embed__wrapper'
);
const borderProps = useBorderProps( attributes );

// Disabled because the overlay div doesn't actually have a role or functionality
// as far as the user is concerned. We're just catching the first click so that
// the block can be selected without interacting with the embed preview that the overlay covers.
/* eslint-disable jsx-a11y/no-static-element-interactions */
const embedWrapper =
'wp-embed' === type ? (
<WpEmbedPreview html={ html } />
<WpEmbedPreview
html={ html }
className={ borderProps.className }
style={ { ...borderProps.style } }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about these new props. Would it be more forward-compatible for them to be combined into something like wrapperProps? It might also help communicate the intended use for the class name and style.

It might also help get other reviewers if we can update the test instructions to cover this scenario as well as others (e.g. explaining to test with a WordPress post as an embed, youtube video, tweet, spotify link etc.) Even better if you could include some test URLs 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in last commit.

/>
) : (
<div className="wp-block-embed__wrapper">
<div
className={ clsx(
'wp-block-embed__wrapper',
borderProps.className
) }
style={ { ...borderProps.style } }
>
<SandBox
html={ html }
scripts={ scripts }
Expand Down
10 changes: 9 additions & 1 deletion packages/block-library/src/embed/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
RichText,
useBlockProps,
__experimentalGetElementClassName,
__experimentalGetBorderClassesAndStyles as getBorderClassesAndStyles,
} from '@wordpress/block-editor';

export default function save( { attributes } ) {
Expand All @@ -18,6 +19,7 @@ export default function save( { attributes } ) {
if ( ! url ) {
return null;
}
const borderProps = getBorderClassesAndStyles( attributes );

const className = clsx( 'wp-block-embed', {
[ `is-type-${ type }` ]: type,
Expand All @@ -27,7 +29,13 @@ export default function save( { attributes } ) {

return (
<figure { ...useBlockProps.save( { className } ) }>
<div className="wp-block-embed__wrapper">
<div
className={ clsx(
'wp-block-embed__wrapper',
borderProps.className
) }
style={ { ...borderProps.style } }
>
{ `\n${ url }\n` /* URL needs to be on its own line. */ }
</div>
{ ! RichText.isEmpty( caption ) && (
Expand Down
3 changes: 3 additions & 0 deletions packages/block-library/src/embed/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
.wp-block-embed {
overflow-wrap: break-word; // Break long strings of text without spaces so they don't overflow the block.

// This block has customizable padding, border-box makes that more predictable.
box-sizing: border-box;

// Supply caption styles to embeds, even if the theme hasn't opted in.
// Reason being: the new markup, figcaptions, are not likely to be styled in the majority of existing themes,
// so we supply the styles so as to not appear broken or unstyled in those.
Expand Down
7 changes: 5 additions & 2 deletions packages/block-library/src/embed/wp-embed-preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const attributeMap = {
marginwidth: 'marginWidth',
};

export default function WpEmbedPreview( { html } ) {
export default function WpEmbedPreview( { html, className, style } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted in my earlier comment, these new props might be better combined into a single wrapperProps prop or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in last commit.

const ref = useRef();
const props = useMemo( () => {
const doc = new window.DOMParser().parseFromString( html, 'text/html' );
Expand Down Expand Up @@ -68,7 +68,10 @@ export default function WpEmbedPreview( { html } ) {
}, [] );

return (
<div className="wp-block-embed__wrapper">
<div
className={ `wp-block-embed__wrapper ${ className }` }
style={ style }
>
<iframe
ref={ useMergeRefs( [ ref, useFocusableIframe() ] ) }
title={ props.title }
Expand Down
Loading