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

Block Bindings: Allow editing in post meta source #61753

Merged
merged 60 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
1ba4af1
Create bindings util for transforming block attributes
SantosGuillamot May 30, 2024
616eafa
Get attributes and name from the store
SantosGuillamot May 30, 2024
8bdd701
Change function to return only the bound attributes
SantosGuillamot May 30, 2024
10b6284
Add action, selector, and reducer for block context
SantosGuillamot May 30, 2024
faf2b09
Sync store in edit component
SantosGuillamot May 30, 2024
65145f1
Revert "Sync store in edit component"
SantosGuillamot May 30, 2024
5438b27
Move logic to BlockContextProvider
SantosGuillamot May 30, 2024
0c75a03
Change parent context logic
SantosGuillamot May 30, 2024
b79a952
Use useLayoutEffect
SantosGuillamot May 30, 2024
1540334
Go back to syncing store in edit component
SantosGuillamot May 30, 2024
5779969
WIP: Move bindings logic to `getBlockAttributes`
SantosGuillamot May 30, 2024
fc2b344
WIP: Move bindings setAttributes logic to updateBlockBindings
SantosGuillamot May 30, 2024
40bfba4
Pass only `select` to `getValue` functions
SantosGuillamot May 30, 2024
467db7e
Remove old editor hook
SantosGuillamot May 30, 2024
88333f8
Add fallback to postId until context is ready
SantosGuillamot May 30, 2024
c35eca1
Remove setValue post-meta code
SantosGuillamot May 30, 2024
187480c
Simplify fallback conditional
SantosGuillamot May 30, 2024
49d438a
Change bindings destructuring
SantosGuillamot May 30, 2024
b8b4af8
Check canBindAttribute in updateBlockAttributes
SantosGuillamot May 30, 2024
711d9a6
Update unit tests to expect a dispatch
SantosGuillamot May 30, 2024
44daa6c
Add conditional in block
SantosGuillamot May 30, 2024
03f4805
Don't use `getBlockAttributes` inside `getValue`
SantosGuillamot May 30, 2024
7dda2e9
Revert "Don't use `getBlockAttributes` inside `getValue`"
SantosGuillamot May 30, 2024
ff84c40
Avoid processing bindings recursively
SantosGuillamot May 30, 2024
037dd37
Access context through selector
SantosGuillamot May 30, 2024
747bede
Update getBlockAttributes logic
SantosGuillamot May 30, 2024
cb98f84
Don't use fallbacks
SantosGuillamot May 30, 2024
827d8b4
Add edit value posibility for post meta, add function to check if is …
cbravobernal May 30, 2024
5ac4874
Revert "Add edit value posibility for post meta, add function to chec…
SantosGuillamot May 30, 2024
361273a
Test editing is allowed in paragraph block
SantosGuillamot May 30, 2024
892f501
Test protected fields are not editable
SantosGuillamot May 30, 2024
da6ca34
Revert "Enable parallel processing for PHPCS sniffs (#61700)"
SantosGuillamot May 30, 2024
98bb6c9
Add post meta setValue function
SantosGuillamot May 30, 2024
11b35f9
Update tests to check contenteditable
SantosGuillamot May 30, 2024
1b7ca45
Update lockAttributesEditing default
SantosGuillamot May 30, 2024
46020f0
Pass arguments to lockAttributesEditing
SantosGuillamot May 30, 2024
1242f65
Check user can edit post meta
SantosGuillamot May 30, 2024
a28ddb9
Check field is exposed in the REST API
SantosGuillamot May 30, 2024
4fcab57
Disable editing in templates
SantosGuillamot May 30, 2024
ff9ffb7
Add fallback for postId
SantosGuillamot May 30, 2024
870e443
Revert "Revert "Enable parallel processing for PHPCS sniffs (#61700)""
SantosGuillamot May 30, 2024
fcca7b1
Adapt old tests
SantosGuillamot May 30, 2024
e2db4a3
Simplify lockAttributesEditing fallbacks
SantosGuillamot May 30, 2024
23c99fc
Don't use fallback for context
SantosGuillamot May 30, 2024
10996f4
Add postType fallback when locking controls
SantosGuillamot May 30, 2024
658e0a3
Pass context in rich text
SantosGuillamot May 30, 2024
46860c7
Check contenteditable attribute in test
SantosGuillamot May 30, 2024
180f0aa
Change name to `canUserEditValue`
SantosGuillamot May 30, 2024
d65b373
Revert changes caused by rebasing
SantosGuillamot May 30, 2024
944f8f2
Add space back
SantosGuillamot May 30, 2024
458a068
Pass block context through rich text
SantosGuillamot May 30, 2024
f7cd1d8
Change imports
SantosGuillamot May 30, 2024
caf6e92
Transform block attributes into bindings in split selection
SantosGuillamot May 30, 2024
5b32ba0
Pass context to in use-input
SantosGuillamot May 30, 2024
ca1296b
Change REST API check
SantosGuillamot May 30, 2024
b5f2acf
Use getBoundAttributesValues
SantosGuillamot May 30, 2024
a9da13e
Don't split when attribute is bound
SantosGuillamot May 30, 2024
4bca305
Revert changes caused by rebase
SantosGuillamot May 30, 2024
f10aa25
Cover more blocks when editing custom fields
SantosGuillamot May 30, 2024
c72afda
Add a warning when pasting blocks
SantosGuillamot May 30, 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: 11 additions & 5 deletions packages/block-editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
useCallback,
forwardRef,
createContext,
useContext,
} from '@wordpress/element';
import { useDispatch, useRegistry, useSelect } from '@wordpress/data';
import { useMergeRefs, useInstanceId } from '@wordpress/compose';
Expand Down Expand Up @@ -39,6 +40,7 @@ import { Content, valueToHTMLString } from './content';
import { withDeprecations } from './with-deprecations';
import { unlock } from '../../lock-unlock';
import { canBindBlock } from '../../hooks/use-bindings-attributes';
import BlockContext from '../block-context';

export const keyboardShortcutContext = createContext();
export const inputEventContext = createContext();
Expand Down Expand Up @@ -121,6 +123,7 @@ export function RichTextWrapper(
const context = useBlockEditContext();
const { clientId, isSelected: isBlockSelected, name: blockName } = context;
const blockBindings = context[ blockBindingsKey ];
const blockContext = useContext( BlockContext );
const selector = ( select ) => {
// Avoid subscribing to the block editor store if the block is not
// selected.
Expand Down Expand Up @@ -170,7 +173,7 @@ export function RichTextWrapper(
const { getBlockBindingsSource } = unlock(
select( blocksStore )
);
for ( const [ attribute, args ] of Object.entries(
for ( const [ attribute, binding ] of Object.entries(
blockBindings
) ) {
if (
Expand All @@ -180,13 +183,16 @@ export function RichTextWrapper(
break;
}

// If the source is not defined, or if its value of `lockAttributesEditing` is `true`, disable it.
// If the source is not defined, or if its value of `canUserEditValue` is `false`, disable it.
const blockBindingsSource = getBlockBindingsSource(
args.source
binding.source
);
if (
! blockBindingsSource ||
blockBindingsSource.lockAttributesEditing()
! blockBindingsSource?.canUserEditValue( {
select,
context: blockContext,
args: binding.args,
} )
) {
_disableBoundBlocks = true;
break;
Expand Down
29 changes: 26 additions & 3 deletions packages/block-editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
} from '@wordpress/blocks';
import { speak } from '@wordpress/a11y';
import { __, _n, sprintf } from '@wordpress/i18n';
import { store as noticesStore } from '@wordpress/notices';
import { create, insert, remove, toHTMLString } from '@wordpress/rich-text';
import deprecated from '@wordpress/deprecated';

Expand Down Expand Up @@ -872,6 +873,30 @@ export const __unstableSplitSelection =
typeof selectionB.attributeKey === 'string'
? selectionB.attributeKey
: findRichTextAttributeKey( blockBType );
const blockAttributes = select.getBlockAttributes(
selectionA.clientId
);
const bindings = blockAttributes?.metadata?.bindings;

// If the attribute is bound, don't split the selection and insert a new block instead.
if ( bindings?.[ attributeKeyA ] ) {
// Show warning if user tries to insert a block into another block with bindings.
if ( blocks.length ) {
const { createWarningNotice } =
registry.dispatch( noticesStore );
createWarningNotice(
__(
"Blocks can't be inserted into other blocks with bindings"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can paste a paragraph inside a binded paragraph to update its content right?

I could do that on my testing, so in that case, we may change the warning to
Blocks can't be insterted into different blocks with bindings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can paste a paragraph inside a binded paragraph to update its content right?

Mmm, I don't think so. You can paste some text, but not if you copy the entire block I believe.

Copy link
Member

Choose a reason for hiding this comment

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

In my testing, it largely depend when you try to paste blocks. For example, try it with the Heading block, the Verse block, or caption for the Image block. It will get handled differently without any notices in the UI:

Screen.Recording.2024-05-31.at.12.54.22.mov

In effect, my recommendation would be to take some action that fits best in the given scenario and skip the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, we probably want to modify the source value with whatever is supposed to be pasted. But that seemed a bit tricky to me at this point and I thought it could be handled in a follow-up PR. That's why I added the warning in the meantime.

),
{
type: 'snackbar',
}
);
return;
}
dispatch.insertAfterBlock( selectionA.clientId );
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have it not do anything tbh

Copy link
Member

Choose a reason for hiding this comment

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

There's also some other nuances: what happens when you paste block into a bound attribute right now with this PR? The correct behaviour would be to paste everything inline imo (since it's an isolated editor)

Copy link
Contributor Author

@SantosGuillamot SantosGuillamot May 29, 2024

Choose a reason for hiding this comment

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

I'd rather have it not do anything tbh

I don't have a strong opinion, to be honest. I think I lean towards adding a new block but I am not sure. Maybe @jasmussen can help to decide. There are basically two options when a block is connected and users press enter:

1. Add a new block

new.block.added.mp4

2. Don't do anything

In this case, maybe we can add a warning saying "Blocks can't be split when using bindings" or something similar.

nothing.happens.mp4

Copy link
Contributor Author

@SantosGuillamot SantosGuillamot May 29, 2024

Choose a reason for hiding this comment

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

There's also some other nuances: what happens when you paste block into a bound attribute right now with this PR? The correct behaviour would be to paste everything inline imo (since it's an isolated editor)

Right now, if you copy text it works as expected, but if you copy a whole block, it does nothing. I agree that probably the expected behavior is to paste everything inline. I'll try to take a look at that. Any clue where that logic lives?

Copy link
Contributor Author

@SantosGuillamot SantosGuillamot May 29, 2024

Choose a reason for hiding this comment

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

I've been testing copying and pasting a block in a heading in trunk, and it seems to be broken, so it is hard to imagine how it should work with bindings. What I see is that after pasting a block, it creates a weird markup:

EDIT: It seems this is solved by this pull request.

Copy.and.paste.bug.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just making sure I understand: we're discussing what happens when you press Enter in a bound block, is that right?

Sorry I missed the comment. Yes, that was my question 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the feedback, I kept the logic to insert a new block when pressing Enter and not allowing to paste blocks when bindings exist until we figure out how to do it properly.

Is that okay for you, @ellatrix ? Any more concerns I should work on to get this merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that shift+enter will add a line break while maintaining the text edition. 👍

Copy link
Contributor

@cbravobernal cbravobernal May 30, 2024

Choose a reason for hiding this comment

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

If we want to be like actual paragraph, all the content from the cursor to the end of the paragraph should be moved to the next block.

PD: I agree that is better to do nothing if we don't have a consensus on the behavior of the enter button here. Also, is of course less prone to errors.

Copy link
Member

Choose a reason for hiding this comment

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

My third suggestion was to insert a soft line break when splitting is not available (which is what rich text does automatically now. Tbh, I don't care that much, as long as we're not splitting the source.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dispatch.insertAfterBlock( selectionA.clientId );

I would remove the enter option, is something we can add in future releases after some UX research.

return;
}

// Can't split if the selection is not set.
if (
Expand Down Expand Up @@ -918,9 +943,7 @@ export const __unstableSplitSelection =
);
}

const length = select.getBlockAttributes( selectionA.clientId )[
attributeKeyA
].length;
const length = blockAttributes[ attributeKeyA ].length;

if ( selectionA.offset === 0 && length ) {
dispatch.insertBlocks(
Expand Down
8 changes: 6 additions & 2 deletions packages/block-library/src/button/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ function ButtonEdit( props ) {
onReplace,
mergeBlocks,
clientId,
context,
} = props;
const {
tagName,
Expand Down Expand Up @@ -246,8 +247,11 @@ function ButtonEdit( props ) {
return {
lockUrlControls:
!! metadata?.bindings?.url &&
( ! blockBindingsSource ||
blockBindingsSource?.lockAttributesEditing() ),
! blockBindingsSource?.canUserEditValue( {
select,
context,
args: metadata?.bindings?.url?.args,
} ),
};
},
[ isSelected, metadata?.bindings?.url ]
Expand Down
7 changes: 5 additions & 2 deletions packages/block-library/src/image/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,11 @@ export function ImageEdit( {
return {
lockUrlControls:
!! metadata?.bindings?.url &&
( ! blockBindingsSource ||
blockBindingsSource?.lockAttributesEditing() ),
! blockBindingsSource?.canUserEditValue( {
select,
context,
args: metadata?.bindings?.url?.args,
} ),
lockUrlControlsMessage: blockBindingsSource?.label
? sprintf(
/* translators: %s: Label of the bindings source. */
Expand Down
21 changes: 15 additions & 6 deletions packages/block-library/src/image/image.js
Original file line number Diff line number Diff line change
Expand Up @@ -462,8 +462,11 @@ export default function Image( {
return {
lockUrlControls:
!! urlBinding &&
( ! urlBindingSource ||
urlBindingSource?.lockAttributesEditing() ),
! urlBindingSource?.canUserEditValue( {
select,
context,
args: urlBinding?.args,
} ),
lockHrefControls:
// Disable editing the link of the URL if the image is inside a pattern instance.
// This is a temporary solution until we support overriding the link on the frontend.
Expand All @@ -474,8 +477,11 @@ export default function Image( {
hasParentPattern,
lockAltControls:
!! altBinding &&
( ! altBindingSource ||
altBindingSource?.lockAttributesEditing() ),
! altBindingSource?.canUserEditValue( {
select,
context,
args: altBinding?.args,
} ),
lockAltControlsMessage: altBindingSource?.label
? sprintf(
/* translators: %s: Label of the bindings source. */
Expand All @@ -485,8 +491,11 @@ export default function Image( {
: __( 'Connected to dynamic data' ),
lockTitleControls:
!! titleBinding &&
( ! titleBindingSource ||
titleBindingSource?.lockAttributesEditing() ),
! titleBindingSource?.canUserEditValue( {
select,
context,
args: titleBinding?.args,
} ),
lockTitleControlsMessage: titleBindingSource?.label
? sprintf(
/* translators: %s: Label of the bindings source. */
Expand Down
2 changes: 1 addition & 1 deletion packages/blocks/src/store/private-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,6 @@ export function registerBlockBindingsSource( source ) {
setValue: source.setValue,
setValues: source.setValues,
getPlaceholder: source.getPlaceholder,
lockAttributesEditing: source.lockAttributesEditing,
canUserEditValue: source.canUserEditValue,
};
}
5 changes: 1 addition & 4 deletions packages/blocks/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,10 +381,7 @@ export function blockBindingsSources( state = {}, action ) {
setValue: action.setValue,
setValues: action.setValues,
getPlaceholder: action.getPlaceholder,
lockAttributesEditing: () =>
action.lockAttributesEditing
? action.lockAttributesEditing()
: true,
canUserEditValue: action.canUserEditValue || ( () => false ),
},
};
}
Expand Down
4 changes: 1 addition & 3 deletions packages/editor/src/bindings/pattern-overrides.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,5 @@ export default {
},
} );
},
lockAttributesEditing() {
return false;
},
canUserEditValue: () => true,
};
52 changes: 46 additions & 6 deletions packages/editor/src/bindings/post-meta.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,53 @@ export default {
return args.key;
},
getValue( { registry, context, args } ) {
const postType = context.postType
? context.postType
: registry.select( editorStore ).getCurrentPostType();

return registry
.select( coreDataStore )
.getEditedEntityRecord( 'postType', postType, context.postId )
.meta?.[ args.key ];
.getEditedEntityRecord(
'postType',
context?.postType,
context?.postId
).meta?.[ args.key ];
},
setValue( { registry, context, args, value } ) {
registry
.dispatch( coreDataStore )
.editEntityRecord( 'postType', context?.postType, context?.postId, {
meta: {
[ args.key ]: value,
},
} );
},
canUserEditValue( { select, context, args } ) {
Copy link
Member

Choose a reason for hiding this comment

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

This is all private right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, registering block binding sources is still private. If everything goes well in WP 6.6 cycle, we are going to open it to everyone in WP 6.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and we will use next cycle to polish the APIs. There are some changes I would like to explore.

const postType =
context?.postType || select( editorStore ).getCurrentPostType();

// Check that editing is happening in the post editor and not a template.
if ( postType === 'wp_template' ) {
return false;
}

// Check that the custom field is not protected and available in the REST API.
const isFieldExposed = !! select( coreDataStore ).getEntityRecord(
'postType',
postType,
context?.postId
)?.meta?.[ args.key ];

if ( ! isFieldExposed ) {
return false;
}

// Check that the user has the capability to edit post meta.
const canUserEdit = select( coreDataStore ).canUserEditEntityRecord(
'postType',
context?.postType,
context?.postId
);
if ( ! canUserEdit ) {
return false;
}

return true;
},
};
Loading
Loading