Skip to content

Implement image alignment controls #416

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

Merged
aduth merged 3 commits into from
Apr 26, 2017
Merged

Implement image alignment controls #416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e50f25
Implement image alignment controls
aduth Apr 12, 2017
7fd79a1
Control image alignment by encoding block attributes as wrapper props
aduth Apr 26, 2017
0857901
Apply wrapper props as opt-in from block implementation
aduth Apr 26, 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
23 changes: 6 additions & 17 deletions blocks/library/image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,12 @@
/**
* Internal dependencies
*/
import './style.scss';
import { registerBlock, query } from 'api';
import Editable from 'components/editable';

const { attr, children } = query;

/**
* Component returning an image element.
*
* @param {string} props.url Image source
* @param {string} props.alt Image alt
* @param {?string} props.align Optional alignment
* @return {WPElement} Image element
*/
function Image( { url, alt, align = 'none' } ) {
return <img src={ url } alt={ alt } className={ `align${ align }` } />;
}

/**
* Returns an attribute setter with behavior that if the target value is
* already the assigned attribute value, it will be set to undefined.
Expand Down Expand Up @@ -74,11 +63,11 @@ registerBlock( 'core/image', {
],

edit( { attributes, setAttributes, focus, setFocus } ) {
const { url, alt, align, caption } = attributes;
const { url, alt, caption } = attributes;

return (
<figure>
<Image url={ url } alt={ alt } align={ align } />
<figure className="blocks-image">
<img src={ url } alt={ alt } />
{ caption || !! focus ? (
<Editable
tagName="figcaption"
Expand All @@ -93,8 +82,8 @@ registerBlock( 'core/image', {
},

save( { attributes } ) {
const { url, alt, align, caption } = attributes;
const img = <Image url={ url } alt={ alt } align={ align } />;
const { url, alt, caption, align = 'none' } = attributes;
const img = <img src={ url } alt={ alt } className={ `align${ align }` } />;

if ( ! caption ) {
return img;
Expand Down
26 changes: 26 additions & 0 deletions blocks/library/image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
.editor-visual-editor__block[data-type="core/image"] {
&[data-align="left"],
&[data-align="right"] {
// Without z-index, won't be clickable as "above" adjacent content
z-index: 10;
max-width: 370px;
}

&[data-align="left"] {
float: left;
margin-right: 15px;
}

&[data-align="right"] {
float: right;
margin-left: 15px;
}
}

.blocks-image {
margin: 0;

img {
display: block;
}
}
21 changes: 21 additions & 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 @@ -3,6 +3,7 @@
*/
import { connect } from 'react-redux';
import classnames from 'classnames';
import { reduce } from 'lodash';

/**
* Internal dependencies
Expand All @@ -29,6 +30,24 @@ const formattingControls = [
}
];

/**
* Returns an object of block attributes to be assigned as data-* attribute
* props on the block's wrapper element.
*
* @param {Object} attributes Block attributes
* @return {Object} Wrapper element props
*/
export function getBlockAttributesAsProps( attributes ) {
return reduce( attributes, ( result, value, key ) => {
const type = typeof value;
if ( 'string' === type || 'number' === type || 'boolean' === type ) {
result[ 'data-' + key ] = String( value );
}

return result;
}, {} );
}

class VisualEditorBlock extends wp.element.Component {
constructor() {
super( ...arguments );
Expand Down Expand Up @@ -148,6 +167,8 @@ class VisualEditorBlock extends wp.element.Component {
onMouseMove={ this.maybeHover }
onMouseLeave={ onMouseLeave }
className={ className }
data-type={ block.blockType }
{ ...getBlockAttributesAsProps( block.attributes ) }
Copy link
Contributor

@youknowriad youknowriad Apr 26, 2017

Choose a reason for hiding this comment

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

Nice workaround :)

I think we should propose an opt-in behaviour (maybe leave it undocumented until needed) and use it in core blocks. I'm a bit concerned with the too many data-* attributes we'll end up with.

Copy link
Contributor

@youknowriad youknowriad Apr 26, 2017

Choose a reason for hiding this comment

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

In all cases, blocks should be aware of the existence of a wrapper div, thus, I think we should move the getBlockAttributesAsProps function to the block API. Each block could decide to add any attribute to its parent div.

Copy link
Member Author

@aduth aduth Apr 26, 2017

Choose a reason for hiding this comment

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

In all cases, blocks should be aware of the existence of a wrapper div, thus, I think we should move the getBlockAttributesAsProps function to the block API. Each block could decide to add any attribute to its parent div.

Yeah, I considered that, and it makes more sense now that I reflect on how the styles already make it clear that the block must be aware of the wrapper element. Your suggestion also addresses worries of computing and assigning attributes unnecessarily, dealing with complex attribute types (more accurately, not dealing with), and lends more flexibility to the block to define different / additional props to apply.

Copy link
Member Author

@aduth aduth Apr 26, 2017

Choose a reason for hiding this comment

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

I bounced back and forth on a few names, or whether this could somehow be integrated into the edit function itself (via callback, or a special shaped return value including metadata).

How does getEditWrapperProps sound to you?

Copy link
Contributor

@youknowriad youknowriad Apr 26, 2017

Choose a reason for hiding this comment

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

It's a good name for now but could be changed later if we settle on changing the edit name

>
{ ( ( isSelected && ! isTyping ) || isHovered ) && <BlockMover uid={ block.uid } /> }
{ isSelected && ! isTyping &&
Expand Down
30 changes: 30 additions & 0 deletions editor/modes/visual-editor/test/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* External dependencies
*/
import { expect } from 'chai';

/**
* Internal dependencies
*/
import { getBlockAttributesAsProps } from '../block';

describe( 'block', () => {
describe( 'getBlockAttributesAsProps()', () => {
it( 'should return an object of data-* attribute props', () => {
const props = getBlockAttributesAsProps( {
string: 'string',
number: 10,
bool: true,
object: {},
array: [],
undef: undefined
} );

expect( props ).to.eql( {
'data-string': 'string',
'data-number': '10',
'data-bool': 'true'
} );
} );
} );
} );
18 changes: 9 additions & 9 deletions languages/gutenberg.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ msgid "Embed"
msgstr ""

#: blocks/library/embed/index.js:31
#: blocks/library/image/index.js:85
#: blocks/library/image/index.js:74
msgid "Write caption…"
msgstr ""

Expand All @@ -24,29 +24,29 @@ msgstr ""
msgid "Heading %s"
msgstr ""

#: blocks/library/image/index.js:36
#: blocks/library/image/index.js:25
msgid "Image"
msgstr ""

#: blocks/library/image/index.js:52
#: blocks/library/image/index.js:41
#: blocks/library/list/index.js:25
#: blocks/library/text/index.js:23
msgid "Align left"
msgstr ""

#: blocks/library/image/index.js:58
#: blocks/library/image/index.js:47
#: blocks/library/list/index.js:33
#: blocks/library/text/index.js:31
msgid "Align center"
msgstr ""

#: blocks/library/image/index.js:64
#: blocks/library/image/index.js:53
#: blocks/library/list/index.js:41
#: blocks/library/text/index.js:39
msgid "Align right"
msgstr ""

#: blocks/library/image/index.js:70
#: blocks/library/image/index.js:59
msgid "No alignment"
msgstr ""

Expand Down Expand Up @@ -102,15 +102,15 @@ msgstr ""
msgid "Publish"
msgstr ""

#: editor/modes/visual-editor/block.js:17
#: editor/modes/visual-editor/block.js:18
msgid "Bold"
msgstr ""

#: editor/modes/visual-editor/block.js:22
#: editor/modes/visual-editor/block.js:23
msgid "Italic"
msgstr ""

#: editor/modes/visual-editor/block.js:27
#: editor/modes/visual-editor/block.js:28
msgid "Strikethrough"
msgstr ""

Expand Down