-
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: Improve the way block bindings sources are registered #63117
Changes from all commits
d716b5f
6f2ab12
6cabef4
4701034
db6f4d4
d776739
d6978d8
efc1450
2a314b1
e8e248c
2c99727
715911d
2866f51
422aef7
c27eed8
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 |
---|---|---|
|
@@ -758,3 +758,178 @@ export const registerBlockVariation = ( blockName, variation ) => { | |
export const unregisterBlockVariation = ( blockName, variationName ) => { | ||
dispatch( blocksStore ).removeBlockVariations( blockName, variationName ); | ||
}; | ||
|
||
/** | ||
* Registers a new block bindings source with an object defining its | ||
* behavior. Once registered, the source is available to be connected | ||
* to the supported block attributes. | ||
* | ||
* @param {Object} source Properties of the source to be registered. | ||
* @param {string} source.name The unique and machine-readable name. | ||
* @param {string} source.label Human-readable label. | ||
* @param {Function} [source.getValue] Function to get the value of the source. | ||
* @param {Function} [source.setValue] Function to update the value of the source. | ||
* @param {Function} [source.setValues] Function to update multiple values connected to the source. | ||
* @param {Function} [source.getPlaceholder] Function to get the placeholder when the value is undefined. | ||
* @param {Function} [source.canUserEditValue] Function to determine if the user can edit the value. | ||
* | ||
* @example | ||
* ```js | ||
* import { _x } from '@wordpress/i18n'; | ||
* import { registerBlockBindingsSource } from '@wordpress/blocks' | ||
* | ||
* registerBlockBindingsSource( { | ||
* name: 'plugin/my-custom-source', | ||
* label: _x( 'My Custom Source', 'block bindings source' ), | ||
* getValue: () => 'Value to place in the block attribute', | ||
* setValue: () => updateMyCustomValue(), | ||
* setValues: () => updateMyCustomValuesInBatch(), | ||
Comment on lines
+784
to
+786
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. Have you decided on the final API? There was discussion about deciding about using with voices in favor of 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 feel confident about using just 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. It wasn't very clear what happened to the open discussions that didn't reach any conclusions. When landing changes, it would be helpful for all observers to explain the action plan for next steps in case they would like to continue following the work. 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 think that's the challenge when documenting the current API that isn't considered a final one. 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 kept them open to leave time for feedback. I wanted to post a new comment with what I consider the conclusion. 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 have just posted this comment and marked it as ready for review. |
||
* getPlaceholder: () => 'Placeholder text when the value is undefined', | ||
* canUserEditValue: () => true, | ||
* } ); | ||
* ``` | ||
*/ | ||
export const registerBlockBindingsSource = ( source ) => { | ||
const { | ||
name, | ||
label, | ||
getValue, | ||
setValue, | ||
setValues, | ||
getPlaceholder, | ||
canUserEditValue, | ||
} = source; | ||
|
||
// Check if the source is already registered. | ||
const existingSource = unlock( | ||
select( blocksStore ) | ||
).getBlockBindingsSource( name ); | ||
if ( existingSource ) { | ||
console.error( | ||
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. Should this be translatable and dev only? Can be done in a follow-up. 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. Maybe? 🤔 I checked the rest of the 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. The most optimal would be: import { warning } from `@wordpress/warning`;
warning( 'This is the message...' ); It's a hint for a developer, but the code continues to work after making the best informed decision. 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 make that change 🙂 I just followed what is being done in the 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 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. That's a good question. Interactivity API uses warnings, whether the interactivity worked or not. 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. It would be great to unify. 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. That makes sense. I will create a pull request to change all the 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. Follow-up created here: #63610. Although I encountered some issues. |
||
'Block bindings source "' + name + '" is already registered.' | ||
); | ||
return; | ||
} | ||
|
||
// Check the `name` property is correct. | ||
if ( ! name ) { | ||
console.error( 'Block bindings source must contain a name.' ); | ||
return; | ||
} | ||
|
||
if ( typeof name !== 'string' ) { | ||
console.error( 'Block bindings source name must be a string.' ); | ||
return; | ||
} | ||
|
||
if ( /[A-Z]+/.test( name ) ) { | ||
console.error( | ||
'Block bindings source name must not contain uppercase characters.' | ||
); | ||
return; | ||
} | ||
|
||
if ( ! /^[a-z0-9/-]+$/.test( name ) ) { | ||
console.error( | ||
'Block bindings source name must contain only valid characters: lowercase characters, hyphens, or digits. Example: my-plugin/my-custom-source.' | ||
); | ||
return; | ||
} | ||
|
||
if ( ! /^[a-z0-9-]+\/[a-z0-9-]+$/.test( name ) ) { | ||
console.error( | ||
'Block bindings source name must contain a namespace and valid characters. Example: my-plugin/my-custom-source.' | ||
); | ||
return; | ||
} | ||
|
||
// Check the `label` property is correct. | ||
if ( ! label ) { | ||
console.error( 'Block bindings source must contain a label.' ); | ||
return; | ||
} | ||
|
||
if ( typeof label !== 'string' ) { | ||
console.error( 'Block bindings source label must be a string.' ); | ||
return; | ||
} | ||
|
||
// Check the `getValue` property is correct. | ||
if ( getValue && typeof getValue !== 'function' ) { | ||
console.error( 'Block bindings source getValue must be a function.' ); | ||
return; | ||
} | ||
|
||
// Check the `setValue` property is correct. | ||
if ( setValue && typeof setValue !== 'function' ) { | ||
console.error( 'Block bindings source setValue must be a function.' ); | ||
return; | ||
} | ||
|
||
// Check the `setValues` property is correct. | ||
if ( setValues && typeof setValues !== 'function' ) { | ||
console.error( 'Block bindings source setValues must be a function.' ); | ||
return; | ||
} | ||
|
||
// Check the `getPlaceholder` property is correct. | ||
if ( getPlaceholder && typeof getPlaceholder !== 'function' ) { | ||
console.error( | ||
'Block bindings source getPlaceholder must be a function.' | ||
); | ||
return; | ||
} | ||
|
||
// Check the `getPlaceholder` property is correct. | ||
if ( canUserEditValue && typeof canUserEditValue !== 'function' ) { | ||
console.error( | ||
'Block bindings source canUserEditValue must be a function.' | ||
); | ||
return; | ||
} | ||
|
||
return unlock( dispatch( blocksStore ) ).addBlockBindingsSource( source ); | ||
}; | ||
|
||
/** | ||
* Unregisters a block bindings source | ||
* | ||
* @param {string} name The name of the block bindings source to unregister. | ||
* | ||
* @example | ||
* ```js | ||
* import { unregisterBlockBindingsSource } from '@wordpress/blocks'; | ||
* | ||
* unregisterBlockBindingsSource( 'plugin/my-custom-source' ); | ||
* ``` | ||
*/ | ||
export function unregisterBlockBindingsSource( name ) { | ||
const oldSource = getBlockBindingsSource( name ); | ||
if ( ! oldSource ) { | ||
console.error( | ||
'Block bindings source "' + name + '" is not registered.' | ||
); | ||
return; | ||
} | ||
unlock( dispatch( blocksStore ) ).removeBlockBindingsSource( name ); | ||
} | ||
|
||
/** | ||
* Returns a registered block bindings source. | ||
* | ||
* @param {string} name Block bindings source name. | ||
* | ||
* @return {?Object} Block bindings source. | ||
*/ | ||
export function getBlockBindingsSource( name ) { | ||
return unlock( select( blocksStore ) ).getBlockBindingsSource( name ); | ||
} | ||
|
||
/** | ||
* Returns all registered block bindings sources. | ||
* | ||
* @return {Array} Block bindings sources. | ||
*/ | ||
export function getBlockBindingsSources() { | ||
return unlock( select( blocksStore ) ).getAllBlockBindingsSources(); | ||
} |
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.
Label should be optional as it can be provided from the server.
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.
I made it optional in the pull request to bootstrap the source from the server: link. Until then, I think label should be required, right?
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.
That works if you have a follow-up lined up.
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.
Yes, there is a follow-up already working. I just need to rebase and it should be ready.