Gallery: Add simple columns slider

[nb]

Allows us to choose columns for a gallery via a slider in the post inspector.

Paired with @mtias on this.

[dmsnell]

Doesn't look like you introduced this, but maybe we should consider using img here for the key instead of the list index, as the list index can lead to bugs when the images end up reordered. The key will be the same but the images different and React will optimize away the change.

[nb]

Thanks, fixed in 6b7bf26, using the url now.

[dmsnell]

Seems like we should synchronize the max here with the max in CSS. That is, if we have an upper limit on CSS why not set the upper limit here…

max={ Math.min( images, length, MAX_CSS_COLUMNS ) }

[nb]

Thanks, should be fixed in d4eb05a.

[nb]

Note: no persistence yet.

[jasmussen]

Nice!

As a future feature it would be nice to consider how this could be made responsive. We could, for example, have one slider for minimum amount of columns, and another for maximum amount of columns, then adjust the number at certain breakpoints.

Or we could show three sliders, one for mobile, one for tablet, one for desktop.

[mtias]

@jasmussen I think that should be handled by the theme, for the most part. Not sure exposing it to the user would be clear enough.

[jasmussen]

👍👍

In any case it would have to be a separate discussion. But good point, and I do see the need to keep the core gallery block simple.

[youknowriad]

Could be a nice improvement to make the slider control generic (the same way we do for text) https://github.com/WordPress/gutenberg/blob/master/blocks/inspector-controls/text-control/index.js

[nb]

Fixed in #1144.

[nb]

Thanks, @paulwilde!

[aduth]

query should always return an array. In what cases was it not?

[nb]

That was caused by my own confusion and I should’ve removed it. Let me explain, because I think other block implementors can fall in the same trap. I wanted to add a default value for the columns attribute. The function that calculates that depends on the number of images and I hoped to avoid the attributes.images = attributes.images || []; in defaultColumnsNumber(). I hadn’t realized however that the attributes property is evaluated only when parsing an existing block and not when creating a new one. So when I got the missing attributes.images on creating a new block this was my first line of defense.

I still haven't internalized where data comes from and the overall attributes flow, starting from a user adding a block, through making changes, persisting, loading, setAttributes, etc.

Have you considered making the block a class, which has the attributes as a member variable and only modify it through setAttributes ala React?

[aduth]

I hadn’t realized however that the attributes property is evaluated only when parsing an existing block and not when creating a new one.

This is an interesting observation, and not one I'd internalized myself. I agree this would be quite confusing.

Attributes initialization in general is an area I think needs a considerable rethink. There's a handful of discussion issues and alternatives explorations that have been tried:

• Discussion: Block attributes initialization #865
• Block parsing and serialization/deserialization #391
• Question: How can we surface expected attributes of a block? #609
• Try: Alternative attributes initialization #892
• Parsing: Declare all block attributes #714
• Add HTML schema for separator block #914

[nb]

While initialization is a concern I think the bigger problems are the lifecycle (what happens when) and the pattern of passing attributes & setAttributes around if we want to do anything slightly more complex.

I still think it would make the most sense the block to be a class with few lifecycle methods. It introduces some state, but the clarity first, and the closeness with React/Vue, second are more important for me. @youknowriad, I know you didn't like the imperative solution, do you think there's a better way?

[youknowriad]

I really think most of the problem is a documentation problem. What does each property of the block settings mean. when it's executed. If we take a step back and take a look at the API, we can split it into three different parts:

1- Parsing attributes from content:

Currently done using the attributes and defaultAttributes setting. There are propositions to use a schema or getInitialAttributes method.

Maybe for clarity, we should simplify this to a parse( commentAttributes, rawContent ) => blockAttributes where rawContent could be a string (like now) or a tree structure (like proposed elsewhere).

2- Editing the attributes:

Once parsed, the block get rendered using the edit Component receiving attributes and setAttributes. I like how this works, I think it's clear enough, maybe I'd just try to find a better name for this settings: form or formComponent. I don't think we should hide the fact that it's a component like we're trying to do now.

3- Serializing attributes to content:

We do this now using the save method. The schema proposed could serve this purpose too, but we're not sure about it yet.

My opinion is that we should make it clear that it's a serialization and that this functions is the opposite of the parse function. serialize( attributes ) => { commentAttributes, rawContent }. Should rawContent here be a string or a tree structure or a React Component. Good question.

---

What I want to say is that we should break our documentation to three steps like this, I think, this makes the API clearer no matter the details.

[aduth]

I still think it would make the most sense the block to be a class with few lifecycle methods.

One of my concerns here is difficulty in extending base classes if a plugin author chooses not to use ES6. This can be seen in React's createClass deprecation, which will now require an additional dependency if developers are bound to it for whatever reason. Class inheritance is not entirely unwieldy in ES5, depending if we need extends vs. plain class.

ES5 class TextBlock:

function TextBlock() {
	// Constructor
}

TextBlock.prototype.edit = function() {
	// Editor form
};

ES5 class TextBlock extends Block:

function TextBlock() {
	Block.apply( this, arguments );

	// Constructor
}

// Extends
TextBlock.prototype = Object.create( Block.prototype );
TextBlock.prototype.constructor = TextBlock;

TextBlock.prototype.edit = function() {
	// Editor form
};

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create#Classical_inheritance_with_Object.create()

[nb]

@aduth wouldn't we need ES6 to make everything work anyway? ES5 would bring other complications, like class variables.

@youknowriad I totally agree that better documentation and naming will go a long way. Speaking of that, looking at the current blocks, there are few two patterns that could be a bit cleaner.

• helper functions need to be both outside of the object – we can’t reference other “methods“ of the block object from edit or save, because they’re run in a totally different context. And they need to often receive attributes and/or setAttributes.
• having full-blown class components for edit and save.

If we don't go the class route, here’s another suggestion: switch the docs (and our current blocks) to have only variable references or short literals in the block object itself and define everything at the top:

registerBlockType( 'core/baba', {
  title: __( 'Title' ),
  attributes,
  edit,
  save: BabaComplicatedSaveComponent, 
}

[aduth]

@aduth wouldn't we need ES6 to make everything work anyway? ES5 would bring other complications, like class variables.

Not necessarily. What did you have in mind? Assigning edit and save as React class components could be challenging, but maybe reasonable at that point to use create-react-class.

[nb]

@aduth if others’ code isn't part of a webpack build how would they import registerBlockType , so that they have access to the API? And if they’re already using webpack, ES6 becomes a reasonable expectation.

[aduth]

@aduth if others’ code isn't part of a webpack build how would they import registerBlockType , so that they have access to the API?

Each entry point (top-level directory) exposes itself on the wp global object:

https://github.com/WordPress/gutenberg/blob/740b93d/webpack.config.js#L27-L31

So by depending on wp-blocks, a plugin author can merely register their block using wp.blocks.registerBlock from the global scope.

Also documented at: https://github.com/WordPress/gutenberg/tree/master/blocks