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.

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

Inserting (pasting/uploading) an image into an empty list item should make it inline #9391

Closed
oleq opened this issue Mar 31, 2021 · 4 comments
Assignees
Labels
package:image squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@oleq
Copy link
Member

oleq commented Mar 31, 2021

📝 Provide a description of the improvement

...unless the pasted image is a block that has a caption already. 

This optimization will preserve the structure of lists, which is pretty fragile and gets easily destroyed when a block image splits it in half.

A follow-up of #9102 and #8659.


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@oleq oleq added type:improvement This issue reports a possible enhancement of an existing feature. package:image squad:core Issue to be handled by the Core team. labels Mar 31, 2021
@oleq oleq added this to the iteration 42 milestone Mar 31, 2021
@AnnaTomanek AnnaTomanek modified the milestones: iteration 42, nice-to-have, iteration 43 Apr 12, 2021
@kuku711
Copy link

kuku711 commented Apr 19, 2021

What about table? this shouldn`t act the same?

@pomek pomek self-assigned this Apr 30, 2021
@pomek
Copy link
Member

pomek commented Apr 30, 2021

I guess to apply the fix properly to resolve the issue is to update the determineImageTypeForInsertionAtSelection() function, which returns the type of image inserted into the editor.

export function determineImageTypeForInsertionAtSelection( schema, selection ) {
const firstBlock = first( selection.getSelectedBlocks() );
return ( !firstBlock || firstBlock.isEmpty || schema.isObject( firstBlock ) ) ? 'image' : 'imageInline';
}

As for now, the inlineImage element will be used when inserting a new image to a non-empty element.

After a few modifications, I can insert the inline image to an empty list item:

export function determineImageTypeForInsertionAtSelection( schema, selection ) {
	const firstBlock = first( selection.getSelectedBlocks() );

	if ( !firstBlock ) {
		return 'image';
	}

	if ( firstBlock.isEmpty ) {
		if ( firstBlock.name === 'listItem' /* || firstBlock.name === 'tableCell' */ ) {
			return 'imageInline';
		}

		return 'image';
	}

	return schema.isObject( firstBlock ) ? 'image' : 'imageInline';
}

However, I am unsure about this solution. What about mentioned the tableCell element or other nodes defined by integrators? How do we want to allow inserting `inline image?

Let's consider two cases:

The first one is about inserting inline images by default into the elements that accept $text nodes.

export function determineImageTypeForInsertionAtSelection( schema, selection ) {
	const firstBlock = first( selection.getSelectedBlocks() );

	// Insert the `image` element if selected any object.
	if ( !firstBlock || schema.isObject( firstBlock ) ) {
		return 'image';
	}

	// Otherwise, check whether the text is allowed in the element. If so, insert an inline image by default.
	return schema.checkChild( firstBlock, '$text') ? 'imageInline' : 'image';
}

The second one is about exposing the configuration array with names of elements where the inline image should be inserted by default.

const config = {
	image: {
		// Yeah, Java style.
		nameOfElementsWithInlineImages: [
			'listItem',
			'tableCell',
			// and more...?
		]
	}
};

export function determineImageTypeForInsertionAtSelection( schema, selection ) {
	const firstBlock = first( selection.getSelectedBlocks() );

	if ( !firstBlock ) {
		return 'image';
	}

	if ( firstBlock.isEmpty ) {
		return nameOfElementsWithInlineImages.includes( firstBlock.name ) ? 'imageInline' : 'image';
	}

	return schema.isObject( firstBlock ) ? 'image' : 'imageInline';
}

As for now, I cannot recall more elements that should insert inline images by default. Maybe I should not complicate the problem and apply the 1st proposition. WDYT?

@niegowski
Copy link
Contributor

The first one is about inserting inline images by default into the elements that accept $text nodes.

This would break the UX that we want to insert block images into an empty block.

The second one is about exposing the configuration array with names of elements where the inline image should be inserted by default.

If we would like to implement this approach then this should be registered from the list feature, not the configuration.

What about mentioned the tableCell element

The table cells should not be our concern, inserting an image is not affecting the table structure (it's not replacing a table cell with a block image).

... or other nodes defined by integrators? 

This one bugs me the most, but OTOH current lists behave this way when there is no inline images support or it's simply disabled so maybe we should go with the simplest hardcoded way at last for now.

niegowski added a commit that referenced this issue May 4, 2021
Other (image): When inserting an image in an empty list item, the `inlineImage` element will be used as the default type of image. Closes #9391.
@niegowski
Copy link
Contributor

Closed in #9606.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

5 participants