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

Allow filtering min and max number of columns in blocks #12861

Closed
wants to merge 1 commit into from

Conversation

swissspidy
Copy link
Member

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:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@swissspidy swissspidy added [Type] Enhancement A suggestion for improvement. [Block] Gallery Affects the Gallery Block - used to display groups of images [Block] Columns Affects the Columns Block labels Dec 13, 2018
@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Dec 14, 2018
@gziolo gziolo requested review from youknowriad and aduth December 14, 2018 09:30
Copy link
Member

@gziolo gziolo left a 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 ),
Copy link
Member

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.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a 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 );
Copy link
Member

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 );
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@swissspidy
Copy link
Member Author

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

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.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 6, 2019
@swissspidy swissspidy closed this Feb 24, 2020
@youknowriad youknowriad deleted the add/column-filters branch May 27, 2020 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block [Block] Gallery Affects the Gallery Block - used to display groups of images [Feature] Extensibility The ability to extend blocks or the editing experience Needs Decision Needs a decision to be actionable or relevant [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit number of columns
4 participants