Use nested blocks for quotes #6054

ellatrix · 2018-04-06T20:49:27Z

Description

Related: #5265.

This branch changes both quotes block so that they use nested blocks for the content. This enables the user to add content other than paragraphs and simplifies the block registration.

~~WIP, to do:~~

~~Preserve citation when unwrapping the blocks.~~
~~Make paste work.~~
~~Double enter should exit the quote.~~ Should this be a general nested block behaviour?
~~Update demo content.~~

How Has This Been Tested?

Ensure old quotes still work (try demo content).
Ensure transformations work.
Try the shortcut: > + SPACE both with and without existing content.

Checklist:

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

ellatrix · 2018-04-06T20:52:36Z

blocks/api/parser.js

@@ -281,7 +281,8 @@ export function createBlockWithFallback( blockNode ) {

 		if ( attributesParsedWithDeprecatedVersion ) {
 			block.isValid = true;
-			block.attributes = attributesParsedWithDeprecatedVersion;
+			block.attributes = omit( attributesParsedWithDeprecatedVersion, '_innerBlocks' );
+			block.innerBlocks = attributesParsedWithDeprecatedVersion._innerBlocks || block.innerBlocks;


This is ugly, but I didn't want to change the deprecation API for this, and I suspect this will be a very rare thing to do. Might still want to change it though.

A similar issue is being resolved in #5932 Let's try to find the best approach there.

youknowriad · 2018-04-10T07:53:59Z

blocks/rich-text/README.md

@@ -31,9 +31,9 @@ a traditional `input` field, usually when the user exits the field.

 *Optional.* By default, a line break will be inserted on <kbd>Enter</kbd>. If the editable field can contain multiple paragraphs, this property can be set to `p` to create new paragraphs on <kbd>Enter</kbd>.

-### `onSplit( before: Array, after: Array, ...blocks: Object ): Function`
+### `onSplit( after: Array, ...blocks: Object ): Function`


Can you elaborate on how this works, it's a bit unclear?

Before, the HTML was splitted between two similar formats (after, and before) but right now it seems that the HTML before the split position is transformed to a block before calling the function (If I understand properly)? How do you decide which block to use since in the RichText component we don't really know the parent block?

Alson If this is the case, why can't we just use onReplace and replace the after with a block as well.

Also, it seems that we need to provide a deprecation message in any case.

Before, the HTML was splitted between two similar formats (after, and before) but right now it seems that the HTML before the split position is transformed to a block before calling the function (If I understand properly)?

No, since the "before" content always lands in the some RichText area, we can do this as part of RichText. I don't thing there's a need to pass it to the block registration, which would only set attributes on the same block affecting the same RichText instance, which already has the correct content. So there's no need to pass as RichText is already setting it, currently the content is being set twice.

Good point about the deprecation message. Me might actually want to improve the shape of the arguments here as it's a bit of a mess. Maybe { blocks, afterValue } or something similar.

Alson If this is the case, why can't we just use onReplace and replace the after with a block as well.

Not sure I'm following here. The block needs to determine what to do with the split off value, which is arguably always a new block with the content set at the same attribute as the "before" value (the value that stays). The problem is that RichText should not be aware of the shape of the block's attributes...

Don't worry about this comment ;) I thought initially you were doing some magic (transforming to blocks) but it's not the case. So my issue right now is more #6054 (comment)

youknowriad · 2018-04-10T07:57:20Z

blocks/rich-text/index.js

 	 *
 	 * @param {Array}  before content before the split position
 	 * @param {Array}  after  content after the split position
 	 * @param {?Array} blocks blocks to insert at the split position
 	 */
 	restoreContentAndSplit( before, after, blocks = [] ) {
-		this.updateContent();
-		this.props.onSplit( before, after, ...blocks );
+		this.setContent( before );


Oh I see, so you apply the content directly beforing calling the onSplit. I believe we avoided this until now because the block can decide to not split and don't support the Enter behavior. If the block don't provide the onSplit it's better to keep content as is instead of removing the part that's after the split point. How do you handle this?

If there's no onSplit prop, restoreContentAndSplit will never be called, just like before? updateContent is being replaced by setContent here, because the only thing we want to do is set the content.

There was actually an inconsistency where some use called setContent before restoreContentAndSplit.

👍 Sounds good to me. Let's try to build a good deprecation strategy though

Will do :) I'll ponder a bit over maybe using onReplace... would be much simpler, but right now onSplit is also used as an indicator whether splitting is possible as you mentioned, whereas onReplace is used for other things too. One solution to both problems might be to have something like this:

onSplit: ( value ) => createBlock ( name, { content: value, }

So here the block provided RichText with the info it needs to split the block, and then we can use onReplace with [ ...blocks, onSplit( value ) ].

But one problem with this is that we currently insert an empty paragraph for the list block if blocks is empty...

Also maybe onSplit always create a defaultBlock for the "after" content in which case, we can replace it entirely?

This need will go away though once we use innerBlocks for the list block too...

Atm I'm tempted to continue passing the before value, but remove the setAttributes, then revise the whole function once we work on lists.

Fine to leave for another refactor though 👍

Cool, let's take care of this when we handle lists.

youknowriad · 2018-04-10T07:59:58Z

editor/components/block-list/block.js

+				insertBlocks( blocks, orderOfRoot + 1, rootUIDOfRoot );
+			} else {
+				insertBlocks( blocks, order + 1, rootUID );
+			}


Can you explain the need of this separate handler and its logic, not obvious at first sight :)

Oh sure! Forgot to comment. It's so escape the nested block list on double enter.

A small e2e test for the double enter behavior might be good. It's something that can regress easily without noticing.

youknowriad · 2018-04-10T08:01:32Z

editor/components/block-settings-menu/block-unwrap.js

@@ -0,0 +1,69 @@
+/**


Can we leave unwrapping to a separate PR. Not saying we shouldn't do it, but it seems better (at least for me) to discuss separately?

Sure, but how do I take care of the block transforms in this PR? Or should I only add the unwrapping if the block has specified a transform (vs always offering unwrap if the block has innerBlocks)?

This is a new feature in this PR right? Is it something needed to replace an existing feature? I suspect it's here to replace some transforms that are not possible in the nested approach? Maybe we don't care that much about these transforms at the moment?

Yes, it's to take care of quote => paragraph/heading. See https://github.com/WordPress/gutenberg/pull/6054/files#diff-cda27cec5557c5b032f2b19c44309a1dL106. I added the unwrapping to show it's easier to take care of these kind of transformations. But I agree this should maybe be discussed more. Are you fine with dropping these transformations then at the moment?

Okay I removed this for now.

youknowriad

Nice work on this PR. I left some comments but it's great to see how these blocks become way simpler now.

afercia · 2018-04-10T09:06:03Z

Sorry I haven't followed so much so maybe I'm missing something, in that case please ignore my question. Could you please specify what you mean by:

This enables the user to add content other than paragraphs

Worth noting the blockquote element is meant to be used for... quotes, that is textual content which typically needs just paragraphs and, optionally, a footer or cite element for the attribution.

youknowriad · 2018-04-10T09:09:35Z

@afercia With #5452 we'll be able to limit the allowed block types for nested blocks per parent block. Also, people wanted to be able to add headings and lists in quotes.

afercia · 2018-04-10T09:17:43Z

@youknowriad that was my concern...

people wanted to be able to add headings and lists in quotes

That sounds totally weird to me. Quotes are not meant to be used for visual purposes. A blockquote element is the representation of a quote, nothing more, nothing less. Though headings and lists in a blockquote seems to be valid HTML, seems to me they're not semantic at all. WordPress aims to be a semantic publishing platform and one of its first goals is to produce rich, semantic, HTML. I'd suggest to wait a bit with this implementation and consider what we're really doing here from a semantics perspective.

In the meantime, I'd suggest to have a look at the specification and all the HTML examples there: https://www.w3.org/TR/html52/grouping-content.html#the-blockquote-element

ellatrix · 2018-04-10T09:24:50Z

@afercia It should be possible to use lists and other blocks in a quote block. I should be able to quote this issue:

WIP, to do:

~~Preserve citation when unwrapping the blocks.~~

~~Make paste work.~~

~~Double enter should exit the quote.~~ Should this be a general nested block behaviour?

~~Update demo content.~~

This is also possible currently in the classic editor and elsewhere (e.g. Markdown).

This PR is also about more than just allowing other blocks in the quote. It's also about simplifying the way the nested paragraphs are implemented. If we end up deciding only paragraphs should be allowed in quotes, we can restrict the blocks that are allowed in a quote.

afercia · 2018-04-10T09:31:13Z

@iseulde sorry that's not a quote in a semantic sense as described in the HTML specification :) That's something more similar to a reply with the original message attached and indented as email clients do with >.

Also, people wanted to be able to add headings and lists in quotes.

It would be great to have access to this feedback and see what are the real use cases users wish to have.

ellatrix · 2018-04-10T09:36:41Z

sorry that's not a quote in a semantic sense as described in the HTML specification :)

Why not? Could you elaborate?

The blockquote element represents content that is quoted from another source, optionally with a citation which must be within a footer or cite element, and optionally with in-line changes such as annotations and abbreviations.

I should be able to quote content form another source, including its semantic structure.

afercia · 2018-04-10T09:37:11Z

@iseulde That's not to say a block for your use case can't be done but then it shouldn't use a blockquote element :) that would be just a representation of a "conversation". From the specification:

The blockquote element represents content that is quoted from another source, optionally with a citation which must be within a footer or cite element, and optionally with in-line changes such as annotations and abbreviations.

I'd point out the part "quoted from another source".

See also the NOTE about how to represent a conversation:

Examples of how to represent a conversation are shown in a later section; it is not appropriate to use the cite and blockquote elements for this purpose.

Examples here: https://www.w3.org/TR/html52/common-idioms-without-dedicated-elements.html#conversations

youknowriad · 2018-04-10T09:40:39Z

@afercia According to the link you shared. The content of the blockquote allows any "Flow Content" element which means any of these elements: https://www.w3.org/TR/html52/dom.html#flow-content-2

This is also confirmed by other sources like https://developer.mozilla.org/en-US/docs/Web/HTML/Element/blockquote

rianrietveld · 2018-04-10T09:41:24Z

According to the specs at MDN, a quote can contain flow content, so headings and list and such are allowed.

Looking at the actual meaning and good use of a blockquote, resticting this to only a p, footer, cite would be more relevant, but also the W3C specs allow flow content.

But for a CMS like WordPress where a blockquote is used already in so many ways, forcing this on content managers, out of pure semantics, seem a bridge too far to me, also for backwards compatibility of text in older posts.

aristath · 2018-04-10T09:41:33Z

While the purist in me agrees 100% that quotes are - and should be - quotes and nothing but quotes, in real-life I've seen lots of sites using quotes to make something stand out. I assume the reason for this is because lots of themes just make quotes pretty and visually appealing. Is it wrong? definitely. Do people still use it just to make a visual impression? Unfortunately yes.

afercia · 2018-04-10T09:41:46Z

@youknowriad as I've previously commented, yeah it seems to be valid HTML. However, my point is about semantics. See the above comments and the NOTE on the specification.

afercia · 2018-04-10T09:43:07Z

I've seen lots of sites using quotes to make something stand out

That's a perfect use case for another block type that doesn't use the blockquote element 🙂

ellatrix · 2018-04-10T09:43:53Z

I'm not representing conversion. I want to quote a source.

E.g.

Primary aromatic amines are used as a starting material for the manufacture of azo dyes. It reacts with nitrous acid to form diazonium salt, which can undergo coupling reaction to form an azo compound. As azo-compounds are highly coloured, they are widely used in dyeing industries, such as:

Methyl orange

Direct brown 138

Sunset yellow FCF

Ponceau

— Some article

afercia · 2018-04-10T09:45:25Z

@iseulde yeah, seems a valid edge case. Question: how many users will abuse this block and use it just for visual purposes?

pento · 2018-04-10T09:48:52Z

So help me, I will turn this car around if you kids don't stop arguing. 😉

ellatrix · 2018-04-10T09:49:04Z

The block clearly says it's a quote, we can even elaborate in the description. You can misuse any block for that. We have to trust the user somewhere. It's more likely they will misuse headings for big text...

Not that it is a good argument in every situation, but we also allowed this content in the classic editor, and it's semantic and valid HTML.

aristath · 2018-04-10T09:51:16Z

Question: how many users will abuse this block and use it just for visual purposes?

I'm guessing that depends on the theme they use, but I don't think it will be any different than what we have now... Those that abuse it will continue to abuse it, and if we add a description that elaborates a bit more like @iseulde suggested maybe some of them will understand and stop. I don't think there's anything more that can - or should - be done.

ellatrix · 2018-04-17T12:41:33Z

Rebased (and squashed).

youknowriad · 2018-04-18T07:16:51Z

test/e2e/support/utils.js

+	await button.click( 'button' );
+}
+
+export async function getHTMLFromCodeEditor() {


Nice refactoring, we talked about having a special jest asserter, but this is nice as well.

youknowriad

LGTM 👍 Nice job

youknowriad · 2018-04-18T07:19:40Z

I was wondering if we should wait for the quote PR to limit the allowed blocks before merging.

81aec55 Don't use rawHandler for migration 188e4a8 Make paste work 86770b7 Adjust e2e, multi selection does not work across nested levels 23027a7 Preserve citation when unwrapping the blocks 188150c Update demo content 2900215 ENTER twice should remove nesting f69f171 Avoid updateContent call on split a54b12d Restore before arg in onSplit, refactor later bf8c171 Handle block (un)wrapping in a separate PR 8929b5f Add e2e test for nested block splitting ef09336 Commits on Apr 12, 2018 Use moveBlockToPosition 6662c36 Fix TinyMCE error on double enter split 6ab8eea Fix split and merge ec7f609

ellatrix · 2018-04-23T22:52:20Z

Rebased. I'm going to merge, we can limit the blocks allowed once the other PR is merged.

This reverts commit 853900d.

ZebulanStanphill · 2018-04-30T14:17:49Z

Should this be removed from the 2.8 milestone since it got reverted?

mcsf · 2018-04-30T14:28:00Z

I've instead added both this and its revert to the milestone, so we have history of this work.

ellatrix · 2018-04-30T16:40:14Z

Thanks! :)

This reverts commit 230ad0d.

ellatrix mentioned this pull request Apr 6, 2018

Simplify quote values #5265

Closed

3 tasks

ellatrix commented Apr 6, 2018

View reviewed changes

ellatrix force-pushed the try/quote-nested-blocks branch 3 times, most recently from f967804 to de6c04b Compare April 9, 2018 19:37

ellatrix requested review from youknowriad, aduth and a team April 9, 2018 21:27

youknowriad reviewed Apr 10, 2018

View reviewed changes

ellatrix force-pushed the try/quote-nested-blocks branch from 1429a3b to 6662c36 Compare April 12, 2018 14:01

ellatrix added this to the 2.7 milestone Apr 12, 2018

ellatrix mentioned this pull request Apr 13, 2018

Fixed empty quote block issue on copy paste #6160

Closed

3 tasks

ellatrix force-pushed the try/quote-nested-blocks branch from ec7f609 to feb649f Compare April 17, 2018 12:40

youknowriad reviewed Apr 18, 2018

View reviewed changes

youknowriad approved these changes Apr 18, 2018

View reviewed changes

youknowriad modified the milestones: 2.7, 2.8 Apr 18, 2018

This was referenced Apr 19, 2018

RichText: Add the RichText.Content component to be used in conjunction with RichText #6274

Merged

Copy/paste issue did not fill out the quote block #5946

Closed

ellatrix force-pushed the try/quote-nested-blocks branch from feb649f to efcadd4 Compare April 23, 2018 22:15

ellatrix force-pushed the try/quote-nested-blocks branch from efcadd4 to 1924ae2 Compare April 23, 2018 22:40

ellatrix merged commit 853900d into master Apr 23, 2018

ellatrix deleted the try/quote-nested-blocks branch April 23, 2018 22:52

jasmussen mentioned this pull request Apr 25, 2018

Can't add cite in Quote block #6404

Closed

nuzzio pushed a commit to nuzzio/gutenberg that referenced this pull request Apr 25, 2018

Use nested blocks for quotes (WordPress#6054)

990dde4

ellatrix added a commit that referenced this pull request Apr 30, 2018

Revert "Use nested blocks for quotes (#6054)"

5389f39

This reverts commit 853900d.

ellatrix added a commit that referenced this pull request Apr 30, 2018

Revert "Use nested blocks for quotes (#6054)" (#6501)

230ad0d

This reverts commit 853900d.

ellatrix added a commit that referenced this pull request May 1, 2018

Revert "Revert "Use nested blocks for quotes (#6054)" (#6501)"

b928ac9

This reverts commit 230ad0d.

ellatrix mentioned this pull request May 1, 2018

Use nested blocks for quotes #6520

Closed

4 tasks

ellatrix added a commit that referenced this pull request May 1, 2018

Revert "Revert "Use nested blocks for quotes (#6054)" (#6501)"

7f666e3

This reverts commit 230ad0d.

noisysocks mentioned this pull request Aug 24, 2018

WIP/Try: Make pullquote a variation #8821

Closed

Use nested blocks for quotes #6054

Use nested blocks for quotes #6054

Conversation

ellatrix commented Apr 6, 2018 • edited Loading

Description

How Has This Been Tested?

Checklist:

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 Apr 10, 2018 • 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

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

youknowriad left a comment

Choose a reason for hiding this comment

afercia commented Apr 10, 2018

youknowriad commented Apr 10, 2018

afercia commented Apr 10, 2018

ellatrix commented Apr 10, 2018

afercia commented Apr 10, 2018

ellatrix commented Apr 10, 2018

afercia commented Apr 10, 2018

youknowriad commented Apr 10, 2018

rianrietveld commented Apr 10, 2018 • edited Loading

aristath commented Apr 10, 2018

afercia commented Apr 10, 2018

afercia commented Apr 10, 2018

ellatrix commented Apr 10, 2018

afercia commented Apr 10, 2018

pento commented Apr 10, 2018

ellatrix commented Apr 10, 2018 • edited Loading

aristath commented Apr 10, 2018

ellatrix commented Apr 17, 2018

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad commented Apr 18, 2018

ellatrix commented Apr 23, 2018

ZebulanStanphill commented Apr 30, 2018

mcsf commented Apr 30, 2018

ellatrix commented Apr 30, 2018

ellatrix commented Apr 6, 2018 •

edited

Loading

ellatrix Apr 10, 2018 •

edited

Loading

rianrietveld commented Apr 10, 2018 •

edited

Loading

ellatrix commented Apr 10, 2018 •

edited

Loading