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.

Sign up for GitHub

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

Gallery: Add simple columns slider #1135

Merged
merged 12 commits into from
Jun 11, 2017
Merged

Gallery: Add simple columns slider #1135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a3fc90d
Add simple columns slider
nb Jun 11, 2017
d4c251b
Enable wide and full-width for galleries.
mtias Jun 11, 2017
6df86e3
Adjust display of label in inspector.
mtias Jun 11, 2017
b906822
We don’t need a key, since it’s not part of a list
nb Jun 11, 2017
6b7bf26
Switch to using URL for key
nb Jun 11, 2017
73fd306
Move default columns logic
nb Jun 11, 2017
c42cf12
Persist columns number as part of attributes
nb Jun 11, 2017
d4eb05a
Limit columns to 8
nb Jun 11, 2017
bf13196
Fix type errors
nb Jun 11, 2017
b6b1093
Remove extra curly braces
nb Jun 11, 2017
4aa3e37
Update gallery fixtures
nb Jun 11, 2017
00ecd81
Add a `for` attribute for the input range
nb Jun 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion blocks/library/gallery/gallery-image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

export default function GalleryImage( props ) {
return (
<figure key={ props.i } className="blocks-gallery-image">
<figure className="blocks-gallery-image">
<img src={ props.img.url } alt={ props.img.alt } />
</figure>
);
Expand Down
67 changes: 48 additions & 19 deletions blocks/library/gallery/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
/**
* Internal dependencies
*/
import { __ } from 'i18n';
import './style.scss';
import { registerBlockType, query as hpq } from '../../api';

import Placeholder from 'components/placeholder';
import MediaUploadButton from '../../media-upload-button';
import InspectorControls from '../../inspector-controls';

import GalleryImage from './gallery-image';

const { query, attr } = hpq;

const MAX_COLUMNS = 8;

const editMediaLibrary = ( attributes, setAttributes ) => {
const frameConfig = {
frame: 'post',
Expand Down Expand Up @@ -51,6 +55,11 @@ function toggleAlignment( align ) {
};
}

function defaultColumnsNumber( attributes ) {
attributes.images = attributes.images || [];
return Math.min( 3, attributes.images.length );
}

registerBlockType( 'core/gallery', {
title: wp.i18n.__( 'Gallery' ),
icon: 'format-gallery',
Expand All @@ -61,15 +70,10 @@ registerBlockType( 'core/gallery', {
query( 'div.blocks-gallery figure.blocks-gallery-image img', {
url: attr( 'src' ),
alt: attr( 'alt' ),
} ),
} ) || [],
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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?

Copy link
Member

Choose a reason for hiding this comment

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

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:

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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()

Copy link
Member Author

Choose a reason for hiding this comment

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

@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, 
}

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

@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

},

controls: [
{
icon: 'format-image',
title: wp.i18n.__( 'Edit Gallery' ),
onClick: editMediaLibrary,
},
{
icon: 'align-left',
title: wp.i18n.__( 'Align left' ),
Expand All @@ -89,16 +93,36 @@ registerBlockType( 'core/gallery', {
onClick: toggleAlignment( 'right' ),
},
{
icon: 'align-full-width',
title: wp.i18n.__( 'Wide width' ),
icon: 'align-wide',
title: __( 'Wide width' ),
isActive: ( { align } ) => 'wide' === align,
onClick: toggleAlignment( 'wide' ),
},
{
icon: 'align-full-width',
title: __( 'Full width' ),
isActive: ( { align } ) => 'full' === align,
onClick: toggleAlignment( 'full' ),
},
{
icon: 'format-image',
title: wp.i18n.__( 'Edit Gallery' ),
onClick: editMediaLibrary,
},
],

edit( { attributes, setAttributes } ) {
const { images, align = 'none' } = attributes;
if ( ! images ) {
getEditWrapperProps( attributes ) {
const { align } = attributes;
if ( 'left' === align || 'right' === align || 'wide' === align || 'full' === align ) {
return { 'data-align': align };
}
},

edit( { attributes, setAttributes, focus, id } ) {
const { images = [], columns = defaultColumnsNumber( attributes ), align = 'none' } = attributes;
const setColumnsNumber = ( event ) => setAttributes( { columns: event.target.value } );
const rangeId = `blocks-gallery-range-${ id }`;
if ( images.length === 0 ) {
const setMediaUrl = ( imgs ) => setAttributes( { images: imgs } );
return (
<Placeholder
Expand All @@ -119,21 +143,26 @@ registerBlockType( 'core/gallery', {
}

return (
<div className={ `blocks-gallery align${ align }` }>
{ images.map( ( img, i ) => (
<GalleryImage key={ i } img={ img } />
<div className={ `blocks-gallery align${ align } columns-${ columns }` }>
{ images.map( ( img ) => (
<GalleryImage key={ img.url } img={ img } />
) ) }
{ focus && images.length > 1 &&
<InspectorControls>
<label className="blocks-text-control__label" htmlFor={ rangeId }>{ __( 'Columns' ) }</label>
<input id={ rangeId } type="range" min="1" max={ Math.min( MAX_COLUMNS, images.length ) } value={ columns } onChange={ setColumnsNumber } />
<span>{columns}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #1144.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @paulwilde!

</InspectorControls> }
</div>
);
},

save( { attributes } ) {
const { images, align = 'none' } = attributes;

const { images, columns = defaultColumnsNumber( attributes ), align = 'none' } = attributes;
return (
<div className={ `blocks-gallery align${ align }` } >
{ images.map( ( img, i ) => (
<GalleryImage key={ i } img={ img } />
<div className={ `blocks-gallery align${ align } columns-${ columns }` } >
{ images.map( ( img ) => (
<GalleryImage key={ img.url } img={ img } />
) ) }
</div>
);
Expand Down
32 changes: 28 additions & 4 deletions blocks/library/gallery/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,41 @@

.blocks-gallery {

display: flex;
flex-wrap: wrap;

.blocks-gallery-image {

.blocks-gallery-image {
flex-grow: 1;
margin: 8px;

img {
max-width: 120px;
max-width: 100%;
}
}

&.columns-1 figure {
width: calc(100% / 1 - 2 * 8px);
}
&.columns-2 figure {
width: calc(100% / 2 - 3 * 8px);
}
&.columns-3 figure {
width: calc(100% / 3 - 4 * 8px);
}
&.columns-4 figure {
width: calc(100% / 4 - 5 * 8px);
}
&.columns-5 figure {
width: calc(100% / 5 - 6 * 8px);
}
&.columns-6 figure {
width: calc(100% / 6 - 7 * 8px);
}
&.columns-7 figure {
width: calc(100% / 7 - 8 * 8px);
}
&.columns-8 figure {
width: calc(100% / 8 - 9 * 8px);
}
}

.blocks-gallery.is-placeholder {
Expand Down
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core-gallery.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<img src="https://cldup.com/uuUqE_dXzy.jpg" alt="title" />
</figure>
<figure class="blocks-gallery-image">
<img src="https://cldup.com/uuUqE_dXzy.jpg" alt="title" />
<img src="http://google.com/hi.png" alt="title" />
</figure>
</div>
<!-- /wp:core/gallery -->
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core-gallery.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"attributes": {
"images": [
{ "url": "https://cldup.com/uuUqE_dXzy.jpg", "alt": "title" },
{ "url": "https://cldup.com/uuUqE_dXzy.jpg", "alt": "title" }
{ "url": "http://google.com/hi.png", "alt": "title" }
]
}
}
Expand Down
4 changes: 2 additions & 2 deletions blocks/test/fixtures/core-gallery.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<!-- wp:core/gallery -->
<div class="blocks-gallery alignnone">
<figure class="blocks-gallery-image"><img src="https://cldup.com/uuUqE_dXzy.jpg" alt="title" /></figure>
<div class="blocks-gallery alignnone columns-2">
<figure class="blocks-gallery-image"><img src="https://cldup.com/uuUqE_dXzy.jpg" alt="title" /></figure>
<figure class="blocks-gallery-image"><img src="http://google.com/hi.png" alt="title" /></figure>
</div>
<!-- /wp:core/gallery -->

1 change: 1 addition & 0 deletions editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ class VisualEditorBlock extends wp.element.Component {
insertBlockAfter={ onInsertAfter }
setFocus={ partial( onFocus, block.uid ) }
mergeBlocks={ this.mergeBlocks }
id={ block.uid }
/>
</div>
</div>
Expand Down