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

Prevent focus loss on reusable block edition #27950

Merged
merged 1 commit into from
Jan 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
52 changes: 16 additions & 36 deletions packages/core-data/src/entity-provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ import {
useContext,
useCallback,
useEffect,
useMemo,
} from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { parse, serialize } from '@wordpress/blocks';

const EMPTY_ARRAY = [];

/**
* Internal dependencies
*/
Expand Down Expand Up @@ -129,56 +130,35 @@ export function useEntityProp( kind, type, prop, _id ) {
* @param {string} kind The entity kind.
* @param {string} type The entity type.
* @param {Object} options
* @param {Object} [options.initialEdits] Initial edits object for the entity record.
* @param {string} [options.blocksProp='blocks'] The name of the entity prop that holds the blocks array.
* @param {string} [options.contentProp='content'] The name of the entity prop that holds the serialized blocks.
* @param {string} [options.id] An entity ID to use instead of the context-provided one.
*
* @return {[WPBlock[], Function, Function]} The block array and setters.
*/
export function useEntityBlockEditor(
kind,
type,
{
initialEdits,
blocksProp = 'blocks',
contentProp = 'content',
id: _id,
} = {}
) {
export function useEntityBlockEditor( kind, type, { id: _id } = {} ) {
const providerId = useEntityId( kind, type );
const id = _id ?? providerId;

const [ content, setContent ] = useEntityProp(
kind,
type,
contentProp,
id
);
const [ content, setContent ] = useEntityProp( kind, type, 'content', id );
const [ blocks, onInput ] = useEntityProp( kind, type, 'blocks', id );

const { editEntityRecord } = useDispatch( 'core' );
useEffect( () => {
if ( initialEdits ) {
editEntityRecord( kind, type, id, initialEdits, {
undoIgnore: true,
} );
}
}, [ id ] );
const initialBlocks = useMemo( () => {
// Load the blocks from the content if not already in state
// Guard against other instances that might have
// set content to a function already.
if ( content && typeof content !== 'function' ) {
const parsedContent = parse( content );
return parsedContent.length ? parsedContent : [];
editEntityRecord(
kind,
type,
id,
{
blocks: parsedContent,
},
{ undoIgnore: true }
);
Copy link
Member

Choose a reason for hiding this comment

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

Howdy @youknowriad 👋 I arrived at this editEntityRecord call when debugging the PostPreviewButton component and how it autosaves the post before showing the preview.

The trouble is that when the isEditedPostAutosaveable selector is called, it calls hasChangedContent, which always returns true even if I didn't edit the post at all.

It's because this editEntityRecord call always adds an edit when the editor is being initialized (more precisely, EditorProvider being mounted).

hasChangedContent checks if there are 'blocks' in edits which is always true. The blocks field never goes away.

What do you think would be a good way to fix this? Is the entity's blocks field also initialized anywhere else, outside EditorProvider? Seems to me the initial value of block should be set on the unedited post entity, to live alongside the original content it is derived from. Not in the edits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't there a flag maybe we can pass to editEntityRecord to mark the change as "non dirty" or something like that? (we had some kind of initial changes in the past).

Copy link
Member

Choose a reason for hiding this comment

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

Currently there isn't. And edits.blocks is not really an edit -- it's a computed property from the original entity.content and there is no changed content. Ideally the *.blocks field would always live on the same object as *.content, as they are two different representations of the same underlying data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have a notion of transientEdits which is what edits.blocks is about but subsequent changes to "blocks" should be considered real edits (mark the post dirty, create undo levels...). Could the first time a "transition edit" be set considered a non-dirty edit?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, blocks and selection edits are considered transient and should never trigger save. Any real edit will also edit content, setting the value to a serialization function to it. So, we can completely ignore blocks and rely on content edits to be always present.

I'm fixing the hasChangedContent selector in #45090 to ignore blocks and look only at content.

}
return [];
}, [ content ] );
const [ blocks = initialBlocks, onInput ] = useEntityProp(
kind,
type,
blocksProp,
id
);

const onChange = useCallback(
( nextBlocks ) => {
Expand All @@ -190,5 +170,5 @@ export function useEntityBlockEditor(
},
[ onInput, setContent ]
);
return [ blocks, onInput, onChange ];
return [ blocks ?? EMPTY_ARRAY, onInput, onChange ];
}
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ describe( 'Reusable blocks', () => {
paragraphBlock.focus();
await pressKeyWithModifier( 'primary', 'a' );
await page.keyboard.press( 'ArrowRight' );
await page.keyboard.type( '*' );
await page.keyboard.type( ' modified' );

// Wait for async mode to dispatch the update.
// eslint-disable-next-line no-restricted-syntax
Expand All @@ -306,7 +306,7 @@ describe( 'Reusable blocks', () => {
'p',
( element ) => element.textContent
);
expect( content ).toEqual( 'Awesome Paragraph*' );
expect( content ).toEqual( 'Awesome Paragraph modified' );
} );
} );
} );
2 changes: 1 addition & 1 deletion packages/editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export function* __unstableSetupTemplate( template ) {
{
blocks,
},
{ __unstableShouldCreateUndoLevel: false }
{ undoIgnore: true }
);
}

Expand Down