Block Switcher: Adding the transformations API and the block switcher #429

youknowriad · 2017-04-15T13:13:37Z

This PR adds a v1 block switcher.

With this proposal a block can define "transformations". A transformation is function transforming the attributes of block of a certain type into another blockType.

This is similar the multi-instance prototype

ellatrix · 2017-04-17T11:44:42Z

I'm not sure about the text block having to define transformations, and transformations between types that are not the text block.

I was thinking it might be best to define transformations to and from the text block when registering a block that can do these, and when we need to transform, we first transform to text, and then from text? So always first to the "base" block.

youknowriad · 2017-04-17T12:06:13Z

I was thinking it might be best to define transformations to and from the text block when registering a block that can do these, and when we need to transform, we first transform to text, and then from text? So always first to the "base" block.

🤔 The advantage of your proposal is that we should think of transformations into a unique block, but it has some drawbacks:

This means we can transform any block to any other block. It's best to limit the scope to similar blocks IMO (could be fixed with a whitelist)
This approach leads to data loss. An attribute could be lost while transforming to text and this attribute can be valuable if we do a direct transformation.
Some blocks can not be transformed to text (blocks including iframes for example)

ellatrix · 2017-04-17T12:37:55Z

This means we can transform any block to any other block. It's best to limit the scope to similar blocks IMO (could be fixed with a whitelist)

I'm not sure what you mean with this. It could work by blocks defining if they can act as a base block, and blocks adding transformations are added to the switcher.

This approach leads to data loss. An attribute could be lost while transforming to text and this attribute can be valuable if we do a direct transformation.

Sure, this will depend on the base block. What kind of data are you thinking about?

Some blocks can not be transformed to text (blocks including iframes for example)

They would not have transformations to text?

youknowriad · 2017-04-17T13:07:47Z

It could work by blocks defining if they can act as a base block, and blocks adding transformations are added to the switcher.

I thought you're talking about having a unique "base block" which could be "text". I don't see the advantage of having multiple base blocks compared to just have targeted transformations (my proposal). In both cases you have to think about multiple blocks.

What kind of data are you thinking about?

I'm not sure, but any data stored as a comment attribute and thus hard to store in a text block.

They would not have transformations to text?

How do they define transformations (for example transforming a Google Map into another Map block)

jasmussen · 2017-04-17T13:17:30Z

From a high level, the block switcher should be able to make only non-destructive transformations. I.e. text to heading, quote, list and back, or a gallery to a slideshow and back.

It would be nice if this were plugin extensible. So a checklist block could register itself as compatible with lists and then show up in the switcher. Or imagine a Podcast player block. You might insert an MP3 and get the built-in Audio block by default, but the Podcast block might have registered itself as compatible with Audio, and let you switch it to that player instead.

I don't know what consequences this has for the discussion above, if any, but wanted to clarify the type of conversions in case it is helpful.

youknowriad · 2017-04-17T13:53:48Z

@iseulde Thinking more about this. and what I like about your proposal is the possibility to define the transformation to a certain block or a transformation from a certain block. I'm starting to think we could be flexible in allowing both. Having two separate keys: transformFrom and transformTo and it's the choice of the block's author to provides the transformations the way he wants to.

Edit With a priority to one direction (maybe transformFrom). Maybe that's exactly what you're proposing and i'm just misunderstanding it :)

aduth · 2017-04-17T16:27:09Z

How does this relate to #413 and some of the ideas for unifying an API to handle not only transformations between types of blocks, but also support for legacy content and shortcodes? Also with regards to desire to enforce always transforming from for sake of extensibility.

youknowriad · 2017-04-17T16:53:23Z

How does this relate to #413 and some of the ideas for unifying an API to handle not only transformations between types of blocks

I'm not sure how these two APIs can be unified. #413 works on HTML content while this transforms block attributes to another type's attributes. I don't see how can we merge those. Maybe you're thinking of transforming the HTML output and parsing it again? Seems like a hack to me.

Also with regards to desire to enforce always transforming from for sake of extensibility.

My first idea was to always enforce the "transform to" but after the discussion with @iseulde I'm thinking we should allow both, that way a block author that wants its block to be switchable from and to a core block, can write both transformations. If we enforce one direction, the custom block can be transformed to (or from) a core block but the not the opposite

aduth · 2017-04-17T17:03:30Z

#413 works on HTML content while this transforms block attributes to another type's attributes.

This is not entirely true, but in fairness #413 lacks good examples that would help clarify this point. The intent there is that to define the equivalent of what you have here with transformations, you could define a compatibility object with a blockType and initialize handler. In this case, the signature would be:

initialize( node: WPBlockNode ): ?Object

In other words, the compatibility layer would always initialize by returning an object of attributes for the new block. The incoming argument could be a block, or a parsed shortcode, or a DOM node.

My point in raising this is more so we don't focus too much on the individual case of supporting block transformations that we overlook the shared characteristics between it and backwards compatibility more generally.

aduth · 2017-04-17T17:06:02Z

that way a block author that wants its block to be switchable from and to a core block, can write both transformations

I agree, this makes sense 👍

youknowriad · 2017-04-17T17:23:38Z

This is not entirely true, but in fairness #413 lacks good examples that would help clarify this point.

I see what you mean now. Thus, I think we can provide this API:

	transformFrom: [
		{
			type: 'block',
			blocks: [ 'core/text' ],
			transform: ( attributes ) => ({})
		},
		{
			type: 'shortcode',
			transform: ( shortcodeAttribiutes ) => ({})
		},
		{
			type: 'dom',
			transform: ( domNode ) => ({})
		},
	],
	transformTo: [
		{
			type: 'block',
			blocks: [ 'core/text' ],
			transform: ( attributes ) => ({})
		}
	],

What do you think?

aduth · 2017-04-17T18:15:48Z

Something like this could work, sure. A few specific remarks:

We'd probably want to offer options to filter more granularly than just dom or shortcode to be very explicit about the types of content which can be transformed.
Could we collapse "from" and "to" into a single property? There's a balance here between becoming too nested with transforms: { from: [ ... ], to: [ ... ] } vs. redundancy on the top-level keys. Maybe even a property of the transform / compatibility itself?

ellatrix · 2017-04-18T07:57:24Z

So a checklist block could register itself as compatible with lists and then show up in the switcher.

Shouldn't a checklist show up as a list variant as part of a list block, just like we have block quote variants and heading variants? In that case the list block would handle transformations to other blocks outside list.

jasmussen · 2017-04-18T08:03:58Z

Shouldn't a checklist show up as a list variant as part of a list block, just like we have block quote variants and heading variants? In that case the list block would handle transformations to other blocks outside list.

By this logic it feels like an ordered list should be a type variant of the unordered list too, which feels like a bridge too far. However your point is salient, and I feel highlights that it's difficult to know exactly where to set the line between what is a block type, and what is a block style.

If a podcast block registers itself as a compatible conversion from the audio block, should it show up as a style choice or a block conversion choice?

I agree, it's not super clean. But for now it feels like we should make the separation at the markup level. A blockquote can have different styles, and it can be losslessly transformed to text and back. Same with ol, and ul. A checklist would probably be a separate block. Down the road you might even imagine a plugin author providing a checklist block that had multiple styles options.

ellatrix · 2017-04-18T08:38:20Z

By this logic it feels like an ordered list should be a type variant of the unordered list too, which feels like a bridge too far.

Not sure if I explained it right, but I meant that ordered list, unordered list, and check list would all be variants/styles of the "higher" list block. Just like heading levels are variants/styles of the heading block and there are variants/styles of a blockquote block.

And yes, this has a lot to do with markup/structure, but nothing with the tag. Heading elements also all have a different tag.

jasmussen · 2017-04-18T08:49:23Z

Understood.

It's a tricky UI, and the question basically boils down to whether we treat the list as the block, and the type of list as a style option for that block (which it sounds like you are proposing). This ties into the freeform block mockup I made:

It's the best UI, arguably, for when you can select across paragraphs. But on the other hand, if it's a style option you'd first have to choose the "list" block in the switcher, and then the style afterwards. This is probably a bit more obtuse if unordered is the default, and you're looking for a way to create an ordered list and don't see the option in the dropdown.

Since list is such a fundamental option, I think it might be worth proceeding with the current plan, with ordered and unordered both showing up in the switcher. This would also allow a plugin to surface different bullet types as style options for the unordered list, and different number/letter/numeral styles for the ordered list block. CC: @intronic and @mimo84 for their work in #382

ellatrix · 2017-04-18T08:56:31Z

Yeah it kind of boils down to how much UI we want to show. If you're looking to create an ordered list, you'd first have to add a list, the block is converted, and then options will pop up to choose a variant. This works the same as headings. Speaking of headings, how would that work in the freeform block mockup?

jasmussen · 2017-04-18T09:17:42Z

Speaking of headings, how would that work in the freeform block mockup?

You'd use the switcher. Even if it's a continuous text field there'd still be block level controls.

youknowriad · 2017-04-18T09:35:44Z

API updated in 6e3dc9d to match #429 (comment)

jasmussen · 2017-04-19T08:56:18Z

UI wise, block switcher works great. I'd like to polish the styles a little more, but I'll do that in a separate PR. Design 👍 from me.

nylen · 2017-04-19T10:01:25Z

blocks/api/factory.js

+	// Find the from transformation
+	const transformation =
+		transformationsFrom.find( t => t.blocks.indexOf( block.blockType ) !== -1 ) ||
+		transformationsTo.find( t => t.blocks.indexOf( blockType ) !== -1 );


I think the transformation to should come first here. This way, if two blocks define conflicting transformations (for example, for a list block, "list to text" and "text from list"), then the current block gets the first shot at the transformation. What do you think?

Yes, I had to make a choice. I remember vaguely discussing this while implementing the per-block prototype and choosing the from transformation as the default but don't remember the arguments though.

Thinking about it more, seems more logic to give priority to the to transformation.

nylen · 2017-04-19T10:05:33Z

editor/modes/visual-editor/block.js

-						isActive: () => control.isActive( block.attributes )
-					} ) ) } />
-			) : null }
+			<div className="editor-visual-editor__block_controls">


Should be editor-visual-editor__block-controls no? (block-controls instead of block_controls)

nylen · 2017-04-19T11:01:05Z

This is working well. Careful merging it, as I did a rebase, which "succeeded" with a bunch of complex git stuff:

First, rewinding head to replay your work on top of it...
Applying: Block Switcher: Addjng the transformations API and the block switcher
Using index info to reconstruct a base tree...
M       blocks/library/text/index.js
M       editor/modes/visual-editor/block.js
M       editor/modes/visual-editor/style.scss
M       editor/state.js
M       editor/test/state.js
Falling back to patching base and 3-way merge...
Auto-merging editor/test/state.js
Auto-merging editor/state.js
Auto-merging editor/modes/visual-editor/style.scss
Auto-merging editor/modes/visual-editor/block.js
Auto-merging blocks/library/text/index.js
Applying: Updating the transformations API to allow transformations from and to a block
Using index info to reconstruct a base tree...
M       blocks/library/heading/index.js
M       blocks/library/text/index.js
Falling back to patching base and 3-way merge...
Auto-merging blocks/library/text/index.js
Auto-merging blocks/library/heading/index.js
Applying: Block Switcher: Giving priority to the "to" transformation

However, the resulting build was broken.

One thing I wanted to test out was what happens when switching a multi-paragraph text block to a heading. IMO this should not be possible, so we will need a way to register some dynamic logic which is able to disable parts of the switcher.

I tried to test this out and found that pressing Enter within a text block does absolutely nothing. We need to get more thorough test cases around actual UX flows as soon as possible.

In any case I would recommend merging this sooner rather than later so I'm going to approve it.

nylen · 2017-04-19T11:06:44Z

@jasmussen: I'm sure you've thought about this, but design-wise, one thing I think is missing here is including the block type icon inline with the up/down arrows. Currently it's not possible to tell what block type you're actually working with. If we add this, the type switcher could just live in the same place, and it would only be enabled if there were actually any blocks available for switching.

… a block

nylen · 2017-04-19T11:16:19Z

the resulting build was broken.

Never mind about this, I just needed to restart npm run dev. Rebasing also mostly fixed the issues I was having with Enter.

I'm going to push up the rebase, but now, when switching a text block to a heading, regardless of how many paragraphs it has, there is no content in the heading.

jasmussen · 2017-04-19T11:23:46Z

I'm sure you've thought about this, but design-wise, one thing I think is missing here is including the block type icon inline with the up/down arrows. Currently it's not possible to tell what block type you're actually working with. If we add this, the type switcher could just live in the same place, and it would only be enabled if there were actually any blocks available for switching.

We actually had this in some of the very first prototypes:

During the prototype phase, though, this did not appear to provide a ton of value to testers, and on the flipside it did cause a fair amount of feedback about too many different places to interact with the block: the switcher on the side, the top docked block level controls, and the inline popup toolbars. And so as part of a unification, we moved it all to be block-docked. This also gave some welcome recognition to what the role of the switcher is, since it essentially became a callback to this:

That's not to say the switcher showing on hover can't work. Absolutely something to keep in mind.

nylen · 2017-04-19T11:25:15Z

That's not to say the switcher showing on hover can't work. Absolutely something to keep in mind.

Then perhaps a smaller tweak would be to always show the block icon as the first icon in the toolbar.

nylen · 2017-04-19T11:25:41Z

when switching a text block to a heading, regardless of how many paragraphs it has, there is no content in the heading.

Fixed in 0c8e6b4, though the fix causes data loss when switching a multi-paragraph text block.

jasmussen · 2017-04-19T11:46:40Z

Then perhaps a smaller tweak would be to always show the block icon as the first icon in the toolbar.

Isn't that implicitly the case with the icon sitting inside the switcher? Or do you mean even on hover?

nylen · 2017-04-19T12:23:57Z

Currently, for a list block for example, no such icon is shown because there are no possibilities for compatible blocks.

nylen · 2017-04-19T12:24:28Z

This will inevitably cause data loss, and it's ok I think. Blocks are different.

This isn't really OK, we need to validate it. However this can be addressed in a separate PR.

aduth · 2017-04-18T16:07:05Z

blocks/library/heading/index.js

@@ -52,5 +52,33 @@ registerBlock( 'core/heading', {
 				style={ align ? { textAlign: align } : null }
 				dangerouslySetInnerHTML={ { __html: content } } />
 		);
+	},
+
+	transforms: {


Minor: Thinking about conventions of property order, I quite like in a typical React component the render is usually the last member of the class, and had thought we could do similar with edit / save, having other properties occur earlier. Do you think there's any value in this or another convention?

Yes, I always forget about this convention sorry. Will fix later

aduth · 2017-04-18T16:09:14Z

blocks/library/heading/index.js

+				blocks: [ 'core/text' ],
+				transform: ( { content, align } ) => {
+					return {
+						tag: 'H2',


As evidenced with new warnings in the React 16 upgrade, we can't render a heading via the capitalized <H2 /> tag name. I guess the easiest option would be to change the edit and save functions to lowercase the tag, which would be fine to do in a separate pull request, unless you had another idea for guaranteeing it in the parse/transform.

or maybe we should store the tag attribute as lowercase

youknowriad · 2017-04-19T12:30:42Z

Yep let's merge this, we have a solid foundation for the transformations API I think

#429 (comment)

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 15, 2017

youknowriad self-assigned this Apr 15, 2017

youknowriad requested a review from aduth April 15, 2017 13:13

youknowriad mentioned this pull request Apr 15, 2017

Add basic heading block #423

Merged

youknowriad force-pushed the add/block-switcher branch from 1e515d3 to eaa8da2 Compare April 17, 2017 12:08

youknowriad changed the title ~~Block Switcher: Addjng the transformations API and the block switcher~~ Block Switcher: Adding the transformations API and the block switcher Apr 17, 2017

aduth mentioned this pull request Apr 17, 2017

Propose block APIs for backwards compatibility #413

Closed

youknowriad mentioned this pull request Apr 18, 2017

Components: Fix the icon button level prop #444

Merged

nylen reviewed Apr 19, 2017

View reviewed changes

nylen approved these changes Apr 19, 2017

View reviewed changes

youknowriad added 3 commits April 19, 2017 13:12

Block Switcher: Addjng the transformations API and the block switcher

c013f29

Updating the transformations API to allow transformations from and to…

3e8b5b5

… a block

Block Switcher: Giving priority to the "to" transformation

9380c38

nylen force-pushed the add/block-switcher branch from 8234662 to 9380c38 Compare April 19, 2017 11:16

Fix switching text blocks to headings

0c8e6b4

aduth reviewed Apr 19, 2017

View reviewed changes

youknowriad merged commit ce8660c into master Apr 19, 2017

youknowriad deleted the add/block-switcher branch April 19, 2017 12:31

nylen added a commit that referenced this pull request Apr 20, 2017

Move heading block transforms to before edit and save

1aed21a

#429 (comment)

Block Switcher: Adding the transformations API and the block switcher #429

Block Switcher: Adding the transformations API and the block switcher #429

Conversation

youknowriad commented Apr 15, 2017

ellatrix commented Apr 17, 2017

youknowriad commented Apr 17, 2017

ellatrix commented Apr 17, 2017

youknowriad commented Apr 17, 2017

jasmussen commented Apr 17, 2017

youknowriad commented Apr 17, 2017 • edited Loading

aduth commented Apr 17, 2017

youknowriad commented Apr 17, 2017

aduth commented Apr 17, 2017

aduth commented Apr 17, 2017

youknowriad commented Apr 17, 2017

aduth commented Apr 17, 2017

ellatrix commented Apr 18, 2017

jasmussen commented Apr 18, 2017 • edited Loading

ellatrix commented Apr 18, 2017

jasmussen commented Apr 18, 2017

ellatrix commented Apr 18, 2017

jasmussen commented Apr 18, 2017

youknowriad commented Apr 18, 2017

jasmussen commented Apr 19, 2017

nylen Apr 19, 2017

Choose a reason for hiding this comment

youknowriad Apr 19, 2017

Choose a reason for hiding this comment

nylen Apr 19, 2017

Choose a reason for hiding this comment

nylen commented Apr 19, 2017

nylen commented Apr 19, 2017 • edited Loading

nylen commented Apr 19, 2017

jasmussen commented Apr 19, 2017

nylen commented Apr 19, 2017

nylen commented Apr 19, 2017 • edited Loading

jasmussen commented Apr 19, 2017

nylen commented Apr 19, 2017

nylen commented Apr 19, 2017

aduth Apr 18, 2017

Choose a reason for hiding this comment

youknowriad Apr 19, 2017

Choose a reason for hiding this comment

aduth Apr 18, 2017

Choose a reason for hiding this comment

youknowriad Apr 19, 2017

Choose a reason for hiding this comment

youknowriad commented Apr 19, 2017

youknowriad commented Apr 17, 2017 •

edited

Loading

jasmussen commented Apr 18, 2017 •

edited

Loading

nylen commented Apr 19, 2017 •

edited

Loading

nylen commented Apr 19, 2017 •

edited

Loading