Implement image alignment controls #416

aduth · 2017-04-12T21:03:14Z

This pull request seeks to implement alignment controls for the image block.

Implementation notes:

Image was extracted to a top-of-file common control to more easily maintain common behaviors between edit and save variations
Parsing align from the className is a bit ugly with the current setup. I'm very inclined to change this to a comment-serialized attribute and support legacy class-based alignment with a compatibility layer like the one explored in Propose block APIs for backwards compatibility #413
Styling is very ugly when performing the alignment

Testing instructions:

Navigate to the Gutenberg admin screen.
Click the first image
Note a toolbar with alignment controls is shown
Note the "No alignment" button is active
Click an alignment button
Note the alignment of the image changes accordingly
Note the clicked alignment button is now active
Click the same button
Note that alignment resets to "No alignment"

jasmussen · 2017-04-13T06:56:36Z

Nice!

Seems like we're missing a few of the niceties from the multi instance prototype. Here's a video:

https://cloudup.com/cCULEOFR95p

Essentially — as soon as the image is floated, the block boundaries look weird on the floated element. Also, the text block takes focus when clicking — I think we did some z-indexing to fix this in the multi instance prototype. @youknowriad do you have more info?

youknowriad · 2017-04-13T09:51:20Z

This is a really hard task IMO. In the current implementation, only the block itself is aware of this attribute. This is problematic because we can not update the styling of the parent .editor-visual-editor__block is not updated properly and I think this is the div that should be floated once we use these controls. Floating this div automattically float its content, and all the attached controls.

Two options here if we want to apply the styling to the container div .editor-visual-editor__block:

Move the rendering of the controls and this div inside the edit of each block. This was the option chosen in the multi-instance prototype. It has the advantage of flexibility because the block author has the control of everything but the problem here is that it adds a lot of things to the block author to think of.
The second option (the one we should consider IMO) is having "global controls". The global controls are controls (and attributes) the editor can be aware of. And these controls can be applied to any blockType no matter its internal implementation. I think the "alignments controls" here (floating) can be applied to any block and thus can be considered "global controls" and we'll have a "reserved attribute name" for these controls.

Edit: There's a third option (an enhancement of the first one) where we can have an optional form property when defining the block. This property is responsible of rendering the surroundings ( the '.editor-visual-editor__blockand its controls ) and calling theeditproperty. If a block don't define theform` attribute, we use the default one (the one we already have).

mtias · 2017-04-13T13:02:12Z

blocks/library/image/index.js

 	},

+	controls: [
+		{


Is the full-bleed or whatever we call that missing?

No, it's not an alignment. It's a width manipulation.

But it's a control.

mtias · 2017-04-13T13:02:42Z

blocks/library/image/index.js

@@ -6,6 +6,22 @@ import Editable from 'components/editable';

 const { attr, html } = query;

+function Image( { url, alt, align } ) {


Probably useful to document here.

mtias · 2017-04-13T13:03:11Z

I'm ok merging this as a first path and exploring the improvements/solutions separately.

aduth · 2017-04-25T20:52:51Z

I've rebased to resolve conflicts and address a few points of the feedback.

Handling the float is particularly challenging, as @youknowriad highlighted earlier. Floated content in general can be difficult, but especially so in our case of trying to shield the block implementation from too much of the editor rendering details. I've tried to hack on a few ideas this afternoon on how the block alone might be able to render its contents as floated while preserving padding, margins, z-index, and border of the surrounding controls, but have struggled to come to any universal solution.

We'd likely need some way for the block listing to become aware that there is floated content. @youknowriad's form property is one idea, but moves significant rendering burden to the block implementation. A few other ideas I've considered:

Recursively iterate the rendered block's children to find if any have a float style applied
Similar to above, simply querySelector the rendered block for [style*="float:"] and assume that the edit render will specify this style
Create a set of generic wrapper elements like <Floated float="left"> and modify the behavior of the list wrapper dependant on its existence
Allow a block to define a callback which returns attributes to be applied to the wrapper
Measure the content and manually draw borders and overlays
Encode the block's attributes into attributes of the block wrapper and apply styles based on their presence (e.g. <div data-type="core/image" data-align="left">)

None of these I'm particularly happy with. Of course, the ideal case is that through some CSS wizardry we could allow the block's content to be floated.

youknowriad · 2017-04-25T21:04:20Z

@aduth there's also the Global controls solution explored in #443

aduth · 2017-04-25T21:27:51Z

@aduth there's also the Global controls solution explored in #443

Yeah, I'd say this is on about a similar level as some of the ideas in my previous comment. The issue with all of these is trying to seek a good balance where neither the block nor the block listing needs to have excessive knowledge about the other. In many of these examples, we're very much targeting alignment/floating as being a common characteristic of a block, and therefore engrain this knowledge into the block list rendering. But... I don't know that we necessarily want to make that assumption.

Another couple ideas to make this generic:

Block list rendering intercepts a style attribute applied to the edit return value and applies it to the wrapper instead (or in addition?)
Following Slot & Fill ideas in Try Slot & Fill pattern for rendering Editable formatting controls #507, maybe the block can opt in to a hook that allows it to specify its own wrapper wrapper

youknowriad · 2017-04-26T17:34:22Z

editor/modes/visual-editor/block.js

@@ -148,6 +167,8 @@ class VisualEditorBlock extends wp.element.Component {
 				onMouseMove={ this.maybeHover }
 				onMouseLeave={ onMouseLeave }
 				className={ className }
+				data-type={ block.blockType }
+				{ ...getBlockAttributesAsProps( block.attributes ) }


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.

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.

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.

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?

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

aduth · 2017-04-26T17:34:26Z

The approach I took in 7fd79a1 is to encode block attributes as data-* attributes of the wrapper element, applying float styles from the image block's stylesheet. I'm not totally sold on this; styles must be aware of existence and class syntax of wrapper, can't encode non-simple types, etc. But I think it's a good compromise to grant some additional wrapper control options to the block.

So now there's float-y goodness:

mtias · 2017-04-26T18:58:55Z

blocks/library/image/index.js

+ * @param  {string}   align Alignment value
+ * @return {Function}       Attribute setter
+ */
+function applyOrUnset( align ) {


Shouldn't this just be toggle? :D

You're too slow @mtias *! But yeah, toggle is appropriate too.

Edit: * Re: Having been merged seconds prior.

aduth added [Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript labels Apr 12, 2017

mtias added the [Feature] Block API API that allows to express the block paradigm. label Apr 13, 2017

mtias reviewed Apr 13, 2017

View reviewed changes

youknowriad mentioned this pull request Apr 18, 2017

Visual Editor: Adding the global controls concept #443

Closed

jasmussen mentioned this pull request Apr 20, 2017

First milestone: Prototype Parity #462

Closed

15 tasks

Implement image alignment controls

3e50f25

aduth force-pushed the update/image-alignment branch from 2793467 to 3e50f25 Compare April 25, 2017 20:52

Control image alignment by encoding block attributes as wrapper props

7fd79a1

ellatrix added this to the Prototype Parity milestone Apr 26, 2017

youknowriad reviewed Apr 26, 2017

View reviewed changes

Apply wrapper props as opt-in from block implementation

0857901

aduth merged commit d2dd9f7 into master Apr 26, 2017

aduth deleted the update/image-alignment branch April 26, 2017 18:58

mtias reviewed Apr 26, 2017

View reviewed changes

aduth mentioned this pull request Apr 26, 2017

Image Block: Add "full width" control option #518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement image alignment controls #416

Implement image alignment controls #416

aduth commented Apr 12, 2017

jasmussen commented Apr 13, 2017

youknowriad commented Apr 13, 2017 •

edited

Loading

mtias Apr 13, 2017

joyously Apr 13, 2017

mtias Apr 13, 2017

mtias Apr 13, 2017

mtias commented Apr 13, 2017

aduth commented Apr 25, 2017 •

edited

Loading

youknowriad commented Apr 25, 2017

aduth commented Apr 25, 2017

youknowriad Apr 26, 2017

youknowriad Apr 26, 2017

aduth Apr 26, 2017

aduth Apr 26, 2017

youknowriad Apr 26, 2017

aduth commented Apr 26, 2017

mtias Apr 26, 2017

aduth Apr 26, 2017 •

edited

Loading

		@@ -6,6 +6,22 @@ import Editable from 'components/editable';

		const { attr, html } = query;

		function Image( { url, alt, align } ) {

Implement image alignment controls #416

Implement image alignment controls #416

Conversation

aduth commented Apr 12, 2017

jasmussen commented Apr 13, 2017

youknowriad commented Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias commented Apr 13, 2017

aduth commented Apr 25, 2017 • edited Loading

youknowriad commented Apr 25, 2017

aduth commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 26, 2017

Choose a reason for hiding this comment

aduth Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

youknowriad commented Apr 13, 2017 •

edited

Loading

aduth commented Apr 25, 2017 •

edited

Loading

aduth Apr 26, 2017 •

edited

Loading