Skip to content

Add sizeSlug to the image inserted from Inserter. #60138

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

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

Add sizeSlug to the image inserted from Inserter. #60138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
79efc8d
add sizeSlug
torounit Mar 22, 2024
6ff6bf0
update unit test
torounit Mar 22, 2024
6ed6c44
use imageDefaultSize
torounit Apr 10, 2024
4aacb3b
fix internal import
torounit Apr 10, 2024
29b6484
fallback to full, if the image of the specified size is not found.
torounit Apr 10, 2024
d1a99fc
set sizeSlug on Media insert.
torounit Apr 10, 2024
5259e2a
rebase trunk
torounit Aug 11, 2024
312160a
remove imageDefaultSize form getBlockAndPreviewFromMedia
torounit Aug 11, 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
28 changes: 27 additions & 1 deletion packages/block-editor/src/components/inserter/media-tab/media-preview.js
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -121,13 +121,14 @@ export function MediaPreview( { media, onClick, category } ) {
useState( false ); useState( false );
const [ isHovered, setIsHovered ] = useState( false ); const [ isHovered, setIsHovered ] = useState( false );
const [ isInserting, setIsInserting ] = useState( false ); const [ isInserting, setIsInserting ] = useState( false );
const { getSettings } = useSelect( blockEditorStore );
Copy link
Contributor

@ntsekouras ntsekouras Apr 19, 2024

Choose a reason for hiding this comment

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

It's good that you extract the selector like this, because we'll call it in a callback function. We could extract the size value from setting though only if we need it, which is when the media category is 'image'.

const { imageDefaultSize } = getSettings();
Copy link
Member

@Mamaduka Mamaduka Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't call the selector during the render. The actual setting value is only needed in the onMediaInsert callback. The sizeSlug does nothing for remote images.

P.S. I've got a small PR that improves the onMediaInsert callback and uses the same technique for the mediaUpload setting - #60983.

const [ block, preview ] = useMemo( const [ block, preview ] = useMemo(
() => getBlockAndPreviewFromMedia( media, category.mediaType ), () => getBlockAndPreviewFromMedia( media, category.mediaType ),
[ media, category.mediaType ] [ media, category.mediaType ]
); );
const { createErrorNotice, createSuccessNotice } = const { createErrorNotice, createSuccessNotice } =
useDispatch( noticesStore ); useDispatch( noticesStore );
const { getSettings } = useSelect( blockEditorStore );


const onMediaInsert = useCallback( const onMediaInsert = useCallback(
( previewBlock ) => { ( previewBlock ) => {
Expand Down Expand Up @@ -169,6 +170,28 @@ export function MediaPreview( { media, onClick, category } ) {
if ( isBlobURL( img.url ) ) { if ( isBlobURL( img.url ) ) {
return; return;
} }

if (
Copy link
Contributor

@ntsekouras ntsekouras Apr 19, 2024

Choose a reason for hiding this comment

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

The check would be better with category.mediaType === 'image' and we could simplify by including the extra attributes in an object and then have only one onClick.

Something like that:

const extraAttributes = {
   id: img.id,
   url: img.url,
};
if (type === image) {
extraAttributes.sizeSlug = ....
}

onClick( {
	...clonedBlock,
	attributes: {
		...clonedBlock.attributes,
		...extraAttributes
	},
} )

Additionally and not related to this PR, we'll need to check how this behaves for other media types besides 'image'. It will most definitely need changes, for example the messages and adding the src attribute instead of the url. This happened because for a while this API wasn't public and we only had Openverse(images).

clonedBlock.name === 'core/image' &&
img.media_type === 'image'
) {
const sizeSlug =
img.sizes?.[ imageDefaultSize ] ||
img.media_details?.sizes?.[
imageDefaultSize
]
? imageDefaultSize
: 'full';
onClick( {
...clonedBlock,
attributes: {
...clonedBlock.attributes,
Copy link
Contributor

@ntsekouras ntsekouras Apr 19, 2024

Choose a reason for hiding this comment

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

Same handling (check media category and add size) should be done above, since I suggested to not set the size in the preview.

The only difference is that we will only need to add the sizeSlug attribute.

id: img.id,
url: img.url,
sizeSlug,
},
} );
} else {
onClick( { onClick( {
...clonedBlock, ...clonedBlock,
attributes: { attributes: {
Expand All @@ -177,6 +200,8 @@ export function MediaPreview( { media, onClick, category } ) {
url: img.url, url: img.url,
}, },
} ); } );
}

createSuccessNotice( createSuccessNotice(
__( 'Image uploaded and inserted.' ), __( 'Image uploaded and inserted.' ),
{ type: 'snackbar' } { type: 'snackbar' }
Expand All @@ -201,6 +226,7 @@ export function MediaPreview( { media, onClick, category } ) {
onClick, onClick,
createSuccessNotice, createSuccessNotice,
createErrorNotice, createErrorNotice,
imageDefaultSize,
] ]
); );


Expand Down
16 changes: 13 additions & 3 deletions packages/block-editor/src/components/inserter/media-tab/media-tab.js
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
@@ -1,10 +1,12 @@
/** /**
* WordPress dependencies * WordPress dependencies
*/ */
import { __ } from '@wordpress/i18n'; import { __, isRTL } from '@wordpress/i18n';
import { useViewportMatch } from '@wordpress/compose'; import { useViewportMatch } from '@wordpress/compose';
import { Button } from '@wordpress/components'; import { Button } from '@wordpress/components';
import { useCallback, useMemo } from '@wordpress/element'; import { useCallback, useMemo } from '@wordpress/element';
import { useSelect } from '@wordpress/data';
import { Icon, chevronRight, chevronLeft } from '@wordpress/icons';


/** /**
* Internal dependencies * Internal dependencies
Expand All @@ -15,8 +17,10 @@ import MediaUpload from '../../media-upload';
import { useMediaCategories } from './hooks'; import { useMediaCategories } from './hooks';
import { getBlockAndPreviewFromMedia } from './utils'; import { getBlockAndPreviewFromMedia } from './utils';
import MobileTabNavigation from '../mobile-tab-navigation'; import MobileTabNavigation from '../mobile-tab-navigation';

import CategoryTabs from '../category-tabs'; import CategoryTabs from '../category-tabs';
import InserterNoResults from '../no-results'; import InserterNoResults from '../no-results';
import { store as blockEditorStore } from '../../../store';


const ALLOWED_MEDIA_TYPES = [ 'image', 'video', 'audio' ]; const ALLOWED_MEDIA_TYPES = [ 'image', 'video', 'audio' ];


Expand All @@ -30,15 +34,21 @@ function MediaTab( {
const mediaCategories = useMediaCategories( rootClientId ); const mediaCategories = useMediaCategories( rootClientId );
const isMobile = useViewportMatch( 'medium', '<' ); const isMobile = useViewportMatch( 'medium', '<' );
const baseCssClass = 'block-editor-inserter__media-tabs'; const baseCssClass = 'block-editor-inserter__media-tabs';
const { getSettings } = useSelect( blockEditorStore );
const { imageDefaultSize } = getSettings();
Copy link
Member

@Mamaduka Mamaduka Apr 23, 2024

Choose a reason for hiding this comment

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

Same here. Let's move the selector call inside the onSelectMedia.

const onSelectMedia = useCallback( const onSelectMedia = useCallback(
( media ) => { ( media ) => {
if ( ! media?.url ) { if ( ! media?.url ) {
return; return;
} }
const [ block ] = getBlockAndPreviewFromMedia( media, media.type ); const [ block ] = getBlockAndPreviewFromMedia(
media,
media.type,
imageDefaultSize
);
onInsert( block ); onInsert( block );
}, },
[ onInsert ] [ onInsert, imageDefaultSize ]
); );
const categories = useMemo( const categories = useMemo(
() => () =>
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/specs/editor/various/inserting-blocks.spec.js
View file
Open in desktop
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -731,8 +731,8 @@ test.describe( 'insert media from inserter', () => {
await page.getByRole( 'tab', { name: 'Images' } ).click(); await page.getByRole( 'tab', { name: 'Images' } ).click();
await page.getByLabel( uploadedMedia.title.raw ).click(); await page.getByLabel( uploadedMedia.title.raw ).click();
await expect.poll( editor.getEditedPostContent ).toBe( await expect.poll( editor.getEditedPostContent ).toBe(
`<!-- wp:image {"id":${ uploadedMedia.id }} --> `<!-- wp:image {"id":${ uploadedMedia.id },"sizeSlug":"full"} -->
<figure class="wp-block-image"><img src="${ uploadedMedia.source_url }" alt="${ uploadedMedia.alt_text }" class="wp-image-${ uploadedMedia.id }"/></figure> <figure class="wp-block-image size-full"><img src="${ uploadedMedia.source_url }" alt="${ uploadedMedia.alt_text }" class="wp-image-${ uploadedMedia.id }"/></figure>
<!-- /wp:image -->` <!-- /wp:image -->`
); );
} ); } );
Expand Down