-
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
Allow filtering min and max number of columns in blocks #12861
Conversation
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.
This is the topic that was discussed a few times on GitHub in the past. There are two things that we haven't solved yet which would impact how this could be solved differently. First of all, we want to expose metadata about blocks to the server, to be able to make customization easier on the server (related discussion in #2751). This would make much easier applying changes to the default values assigned to blocks. At the moment you can do it using hooks
library on the client using blocks.registerBlockType
filter. The other part that has been considered but never implemented is adding new types for attributes. In this particular case, range
type would fit making customization very easy:
attributes: {
columns: {
type: 'range',
default: 2,
min: 2,
max: 6,
},
},
It's easy to add new filters as proposed in this PR but it's going to be hard to scale because we will have to maintain all of them and keep docs up to date. There is also this asynchronous flow involved in the JavaScript involved. It makes everything difficult. To ensure that filters are properly applied you have to load the plugin which adds filters to be executed before the block definition gets loaded. In the past year, we learned that this is super confusing and hard to configure using dependencies
field in the assets you enque. That's why we are constantly seeking for alternative solutions.
@@ -77,7 +99,7 @@ export const settings = { | |||
attributes: { | |||
columns: { | |||
type: 'number', | |||
default: 2, | |||
default: parseInt( DEFAULT_COLUMNS, 10 ), |
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.
This one might be confusing as you are also able to update this value using the existing blocks.registerBlockType
filter.
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.
Hi @swissspidy thank you for your work on this solution. I feel changing minimum or the maximum number of columns in a block is changing an option of that block. Similar to how we may want to change the aria-label of the specific paragraph, or the available align options in an image.
In this approach we change the options for all the blocks of a given type, we should allow a more granular control, allowing the filter to specify in which specific block the option should be applied.
We have an issue to discuss a solution to allow changing block options #7931, and the functionality of this PR probably would fall there.
Personally, I like the hooks and filters approach, and I think one possible solution would be make the edit component receive an optional options prop and then we would be able to change this prop using the already existing editor.__experimentalBlockListBlock filter.
This way we would not be creating new filters and at the same time we would be able to apply specific options per specific block instance, in the method referred by @gziolo we would only be able to specify options for every block of a given type.
* | ||
* @param {Number} number The column count. Defaults to 2. | ||
*/ | ||
const MIN_COLUMNS = applyFilters( 'blocks.gallery.minColumns', 2 ); |
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.
What if I want to change the min columns of a gallery, that has certain attributes or that is nested in a specific block? I feel we should pass more information at least the block client id.
* | ||
* @param {Number} number The column count. Defaults to 6. | ||
*/ | ||
const MAX_COLUMNS = applyFilters( 'blocks.columns.maxColumns', 6 ); |
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 just want to mention that in #core-js chats we agreed to limit the use of hooks and filters. We noticed that for the JavaScript code, hooks and filters are not the best extensibility APIs.
It might be ok for these particular numbers but let's be very careful before adding new ones.
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's very good to know, thanks! Do you happen to have a link to that discussion or a summary of it by chance? I'd be curious to learn more.
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, more context in this summary https://make.wordpress.org/core/2018/11/07/javascript-chat-summary-november-6th/
To be fair, docs should be updated automatically using a docs parser, as is already being done for PHP code in core. And I think for JS there's something in the works already. |
Description
Fixes #10791 by adding some filters to allow changing the minimum and maximum number of columns for both the gallery and the columns block.
Checklist: