Keep and handle image alignment classes for raw transforms #3724

ellatrix · 2017-11-29T22:44:19Z

Description

This PR updates raw handling to preserve WordPress alignment classes and updates the image block transformation to detect the classes and set the align attribute.

How Has This Been Tested?

Paste the following structure:

<p><img src="https://cldup.com/YLYhpou2oq.jpg" alt="Beautiful landscape" class="alignleft" /></p>

This would previously not preserve alignment.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

codecov · 2017-11-29T22:44:22Z

Codecov Report

Merging #3724 into master will decrease coverage by 1.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3724      +/-   ##
==========================================
- Coverage   39.08%   37.96%   -1.12%     
==========================================
  Files         295      278      -17     
  Lines        7016     6806     -210     
  Branches     1290     1247      -43     
==========================================
- Hits         2742     2584     -158     
+ Misses       3588     3557      -31     
+ Partials      686      665      -21

Impacted Files	Coverage Δ
blocks/api/factory.js	`86.48% <ø> (-1.65%)`	⬇️
blocks/api/raw-handling/strip-attributes.js	`100% <100%> (ø)`	⬆️
blocks/api/raw-handling/utils.js	`98.11% <100%> (-0.17%)`	⬇️
blocks/library/image/index.js	`56.52% <100%> (+6.52%)`	⬆️
components/panel/color.js	`0% <0%> (-100%)`	⬇️
blocks/block-controls/index.js	`0% <0%> (-100%)`	⬇️
components/panel/row.js	`0% <0%> (-100%)`	⬇️
editor/components/warning/index.js	`0% <0%> (-100%)`	⬇️
blocks/color-palette/index.js	`0% <0%> (-90.91%)`	⬇️
blocks/alignment-toolbar/index.js	`16.66% <0%> (-83.34%)`	⬇️
... and 131 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecee09e...ec6e5e7. Read the comment docs.

mcsf

This works well for me, but I need to sleep on the block API changes and the whitelisting approach in order to form an opinion. 😄

mcsf · 2017-11-29T23:32:38Z

blocks/api/raw-handling/strip-attributes.js

+
+	const newClasses = oldClasses.split( ' ' ).filter( ( name ) => {
+		return name && isClassWhitelisted( tag, name );
+	} );


For type consistency with oldClasses, the join should happen here:

const newClasses = oldClasses.split( ' ' ).filter( … ).join( ' ' );

also, that filter predicate is a great candidate for implicit return

.filter( ( name ) => name && …

The subsequent newClasses.length would still work (switching from Array#length to String#length).

mcsf · 2017-11-30T12:43:35Z

blocks/api/raw-handling/utils.js

@@ -34,7 +34,7 @@ const inlineWrapperWhiteList = {
 const whitelist = {
 	...inlineWhitelist,
 	...inlineWrapperWhiteList,
-	img: { attributes: [ 'src', 'alt' ] },
+	img: { attributes: [ 'src', 'alt' ], classes: [ 'alignleft', 'aligncenter', 'alignright', 'alignnone' ] },


Side thought: we should start to consider performance and do some measuring. Some of the predicates that consume this data could perhaps benefit from a more barebones switch construct, for instance.

Could you give an example?

Oh, I just had this benchmark in the back of my head.

mcsf · 2017-11-30T12:48:24Z

blocks/library/image/index.js

@@ -70,6 +70,14 @@ registerBlockType( 'core/image', {

 					return tag === 'img' || ( hasImage && ! hasText ) || ( hasImage && tag === 'figure' );
 				},
+				transform: ( node ) => {


Minor suggestion: shorthand transform( node ) {

mcsf · 2017-11-30T12:51:22Z

blocks/library/image/index.js

@@ -70,6 +70,14 @@ registerBlockType( 'core/image', {

 					return tag === 'img' || ( hasImage && ! hasText ) || ( hasImage && tag === 'figure' );
 				},
+				transform: ( node ) => {
+					const targetNode = node.parentNode.querySelector( 'figure,img' );
+					const matches = /align(left|center|right)/.exec( targetNode.className );


matches doesn't mean much in the context of the entire transform. I suggest encapsulating the whole thing such that you get

const align = parseAlignment( node ); // or, if you prefer (I personally don't): const align = parseAlignment( node.parentNode.querySelector( 'figure, img' ) ); return createBlock( …, { align }, targetNode.outerHTML );

I don't really understand the need here to wrap that in another function. Also, the second option would be needed to get targetNode.

I don't feel as strongly as I did when I wrote this. :)

mcsf · 2017-11-30T12:58:24Z

blocks/api/factory.js

+ * @param  {String}  name             Block name
+ * @param  {Object}  blockAttributes  Block attributes
+ * @param  {?String} innerHTML        Block HTML
+ * @return {Object}                   Block object


I'm still undecided on this API change; in any case, I suggest explaining in the jsdoc what the implications of passing that optional innerHTML are, or why one should use it.

mcsf · 2017-11-30T13:01:24Z

blocks/api/factory.js

@@ -52,7 +54,7 @@ export function createBlock( name, blockAttributes = {} ) {
 		uid: uuid(),
 		name,
 		isValid: true,
-		attributes,
+		attributes: innerHTML ? getBlockAttributes( blockType, innerHTML, attributes ) : attributes,


If it helps the discussion, getBlockAttributes is already part of the public interface of @wordpress/blocks; in other words, this change is more about the convenience we offer block authors with this polymorphic version of createBlock rather than keeping some piece of the blocks module private.

mcsf · 2017-11-30T13:08:32Z

blocks/api/factory.js

 */
-export function createBlock( name, blockAttributes = {} ) {
+export function createBlock( name, blockAttributes = {}, innerHTML ) {


Before this change, createBlock meant something very basic: run a simple check against blockType.attributes to grab the default values when values are missing, and return a plain object representing a block. No parsing is involved, etc.

Are we OK with changing that?

Do we prefer adding a dedicated function, e.g. createBlockFromHTML( name, innerHTML, extraAttributes )?

Do we prefer not adding anything and requiring authors to combine createBlock and getBlockAttributes on their own?

I feel bad not mentioning this in the PR description. Yes, there are some options here, and code wise all of them don't require much extra. I first used getBlockAttributes, but then thought it made sense as part of createBlock. A block usually consists of a name, attributes and HTML, so it makes sense to be able to create a block from all that information?

Could simplify things internally as well. In parser.js we do:

const block = createBlock( name, getBlockAttributes( blockType, innerHTML, attributes ) );

So why not:

const block = createBlock( name, attributes, innerHTML );

I can understand where this idea comes from. We never used getBlockAttributes outside the api yet I think.

But I think I'll be in favor of keeping the createBlock simple. And compose the two functions when needed because there's a major difference in the expected "attributes" argument here.

the attributes parameter of createBlock can be any attribute allowed by the block, it behaves like "defaultAttributes" somehow. While the "attributes" parameter of getBlockAttributes represent only the "comment" attributes (the parsed JSON).

See also #3745, conflicting changes with API and intending to pass inner children nodes.

ellatrix · 2017-12-26T20:23:02Z

@mcsf Does this look better? I'm not quite liking how getBlockAttributes needs the blockType instead of just the name. Overall I think one API call would be nice though.

mcsf · 2018-01-02T18:12:43Z

@mcsf Does this look better? I'm not quite liking how getBlockAttributes needs the blockType instead of just the name. Overall I think one API call would be nice though.

I was quite torn, because I agree that it doesn't look great.

Ultimately, I'd say it may be best to err on the conservative side and not touch the createBlock interface, letting it stay conceptually very simple. At the same time, there is now a precedent in the Image block of mixing together "external" attributes (e.g. alignment as inferred in the transform) and "automatic" ones (e.g. values sourced from DOM, default values), and we can reassess the need for an interface change later—ideally, after more third-party block providers have consumed these interfaces.

ellatrix requested a review from mcsf November 29, 2017 22:47

ellatrix mentioned this pull request Nov 29, 2017

User Testing - Pasting Content with HTML #3336

Closed

mcsf reviewed Nov 29, 2017

View reviewed changes

mcsf reviewed Nov 30, 2017

View reviewed changes

ellatrix force-pushed the fix/paste-image-alignment branch from 5d4f9a2 to ec6e5e7 Compare December 26, 2017 12:28

Keep and handle image alignment classes for raw transforms

a22c959

ellatrix force-pushed the fix/paste-image-alignment branch from ec6e5e7 to a22c959 Compare December 26, 2017 13:03

Do not change the API

2a4991d

ellatrix requested a review from mcsf December 31, 2017 14:23

mcsf approved these changes Jan 2, 2018

View reviewed changes

ellatrix merged commit eb7b0b7 into master Jan 3, 2018

ellatrix deleted the fix/paste-image-alignment branch January 3, 2018 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep and handle image alignment classes for raw transforms #3724

Keep and handle image alignment classes for raw transforms #3724

ellatrix commented Nov 29, 2017

codecov bot commented Nov 29, 2017 •

edited

Loading

mcsf left a comment

mcsf Nov 29, 2017

mcsf Nov 30, 2017

ellatrix Nov 30, 2017

mcsf Jan 2, 2018

mcsf Nov 30, 2017

mcsf Nov 30, 2017

ellatrix Dec 26, 2017

mcsf Jan 2, 2018

mcsf Nov 30, 2017

mcsf Nov 30, 2017

mcsf Nov 30, 2017

ellatrix Nov 30, 2017

ellatrix Nov 30, 2017

youknowriad Dec 1, 2017

aduth Dec 4, 2017

ellatrix commented Dec 26, 2017

mcsf commented Jan 2, 2018

Keep and handle image alignment classes for raw transforms #3724

Keep and handle image alignment classes for raw transforms #3724

Conversation

ellatrix commented Nov 29, 2017

Description

How Has This Been Tested?

Checklist:

codecov bot commented Nov 29, 2017 • edited Loading

Codecov Report

mcsf left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Dec 26, 2017

mcsf commented Jan 2, 2018

codecov bot commented Nov 29, 2017 •

edited

Loading