-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
1ba4af1
616eafa
8bdd701
10b6284
faf2b09
65145f1
5438b27
0c75a03
b79a952
1540334
5779969
fc2b344
40bfba4
467db7e
88333f8
c35eca1
187480c
49d438a
b8b4af8
711d9a6
44daa6c
03f4805
7dda2e9
ff84c40
037dd37
747bede
cb98f84
827d8b4
5ac4874
361273a
892f501
da6ca34
98bb6c9
11b35f9
1b7ca45
46020f0
1242f65
a28ddb9
4fcab57
ff9ffb7
870e443
fcca7b1
e2db4a3
23c99fc
10996f4
658e0a3
46860c7
180f0aa
d65b373
944f8f2
458a068
f7cd1d8
caf6e92
5b32ba0
ca1296b
b5f2acf
a9da13e
4bca305
f10aa25
c72afda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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'; | ||||
|
||||
|
@@ -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" | ||||
), | ||||
{ | ||||
type: 'snackbar', | ||||
} | ||||
); | ||||
return; | ||||
} | ||||
dispatch.insertAfterBlock( selectionA.clientId ); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather have it not do anything tbh There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.mp42. 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.mp4There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 EDIT: It seems this is solved by this pull request. Copy.and.paste.bug.mp4There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry I missed the comment. Yes, that was my question 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can confirm that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I would remove the |
||||
return; | ||||
} | ||||
|
||||
// Can't split if the selection is not set. | ||||
if ( | ||||
|
@@ -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( | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,7 +89,5 @@ export default { | |
}, | ||
} ); | ||
}, | ||
lockAttributesEditing() { | ||
return false; | ||
}, | ||
canUserEditValue: () => true, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 } ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is all private right? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}, | ||
}; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, I don't think so. You can paste some text, but not if you copy the entire block I believe.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.