Blocks: Adding the quote block

[youknowriad]

This is a follow-up PR to #327 which tries to add a quote block. refs #311

Props to @BE-Webdesign

[youknowriad]

I wanted to hide this when the citation is empty and we're not focusing the block but it's not possible for now, because the isSelected is not enough for this. We need a distinct isFocused which stays active even if we start typing.

[aduth]

We need a distinct isFocused which stays active even if we start typing.

Should we create an issue to track this, or do you plan to tackle this in the near future?

[youknowriad]

I was planing to take a look at this (but don't know really when) Let's create an issue for this :)

[youknowriad]

#420

[aduth]

Ah yes, the shadow conflict. It's been on my mind lately whether we should rename wp.blocks.query to something else. I thought perhaps parse being the default exported function with other methods as properties available via destructuring, but I'm not sure I like the idea of block implementers thinking of this as a parse action (even if it is 😄 ).

[aduth]

Or we could simply reference hpq as a proper external dependency and expect plugin authors to enqueue it themselves as a preloaded dependency. I quite liked masking the underlying implementation detail and providing a single unified interface for blocks though.

[aduth]

Won't this disallow multiple paragraphs in a single quote? Instead maybe we want to retrieve paragraphs as an array of HTML?

Roughly:

attributes: {
	value: html( 'p' )
}

save( { value } ) {
	return <blockquote>{ value.map( ( paragraph ) => <p>{ paragraph }</p> ) }</blockquote>;
}

[youknowriad]

The above will retrieve all the paragraphs and join them, so no it won't disallow multiple paragraphs. I wanted to have a unique string to pass to the Editable inside the edit instead of having to duplicate the join logic in edit and save.

Also worth noting that the footer can contain p elements, so that's why I'm using blockquote > p

[aduth]

The above will retrieve all the paragraphs and join them, so no it won't disallow multiple paragraphs.

But html retrieves the innerHTML so it would lose the tag and become a single joined string.

I wanted to have a unique string to pass to the Editable inside the edit instead of having to duplicate the join logic in edit and save.

I guess I'm more on the side of using attributes to get the raw-est data possible even if that's not necessarily the most useful, and assume that edit and save will form it into whatever's most applicable, perhaps sharing logic with common components if need be.

Also worth noting that the footer can contain p elements

Is this "can" as in: technically possible per spec, or something we want to support?

[youknowriad]

Good point! What about adding a div wrapper inside the blockquote to separate the footer from the content properly. This will also resolve the dangerouslySetInnerHTML issue?

[youknowriad]

Is this "can" as in: technically possible per spec, or something we want to support?

I don't know :P. What I know is that TinyMCE (current implementation) adds p automatically around text (we could avoid it passing a tagName, but we create other limits by doing so). Also the current post content sample contains the p

[aduth]

Not that it really matters much here, but if it's shown effectively before the cite, should we apply as :before for semantic's sake?

[youknowriad]

👍

[aduth]

I think we'll need some dangerouslySetInnerHTML throughout here.

[youknowriad]

🤔 How do we apply the dangerouslySetInnerHTML without a wrapper?

[aduth]

🤔 How do we apply the dangerouslySetInnerHTML without a wrapper?

Yeah, we ought to be thinking about this, because I don't think it's particularly obvious and the "dangerous" terminology is quite off-putting.

Perhaps we can walk the tree returned from the function and manually revise string children as dangerouslySetInnerHTML? 😬

[youknowriad]

@aduth you're gonna hate it but in 12db3e9 I've used what's currently possible to address the multi-paragraph issue and the dangerouslySetInnerHTML without changing the markup.

(Return a string from the save and do the <p> concat while parsing)

[aduth]

@youknowriad I put together an example of what I was proposing at f25da1a, where the raw attribute value of a quote is an array of the paragraphs' HTML.

I've observed a few sticking points in this conversation I think we'll need to sort through to set good conventions for block implementers:

• Just how much parsing should we expect the implementer to be doing in attributes?
• Should string return values from save be allowed? Seems inevitable if we allow them that they'll be leaned upon heavily. How would we protect against escaping issues if it were to become a preferred approach?
• f25da1a highlights a more desperate need to think of another solution for dangerouslySetInnerHTML in the save function. I've opened an issue at Alternative to dangerouslySetInnerHTML in block save #421 to explore this.

[youknowriad]

@aduth Pros and cons I can see in f25da1a

Pros:

• attributes parsing is simple
• fix the dangerouslySetInnerHTML while still returning an Element

Cons:

• We're parsing the value obtained from the Editable. I think one of the goals of our architecture is to avoid parsing for block authors (aside the attributes)

---

My POV is that we should parse all what can be parsed before hitting the edit method to simplify it the most. You may advocate the opposite? Maybe we should think of this issue in terms for block's state. What's the most logic for a quote block: Having a value as an array of paragraph contents, or having a unique content value. "A quote have a content and a citation" makes more sense to me.

[youknowriad]

I've included your commit @aduth The two approaches have flaws. I do prefer returning only strings from the save method, but we can move forward for now. And see how this performs with other blocks. This is an easy enough change (can be revisited later).

[aduth]

You may advocate the opposite?

I don't necessarily see it as one extreme or the other, but I do think it's reasonable for the block implementer to think of the block's attribute state in terms of what's most appropriate to accommodate the underlying values, ideally with as little markup as possible. If this means they have to parse content on the Editable's onChange handler, I'm fine with that. I think part of the problem is that we're trying to reconcile two competing approaches, one which tries to abstract the block as an object of raw values, and another which is okay with representing some of that state as markup. I have a preference toward the former, but it's complicated by the Editable containing nested nodes, the consequences of which are that we must do more parsing on its change handler and apply dangerouslySetInnerHTML in save.

[aduth]

Looks good 👍

[aduth]

What's the effect we're trying to achieve with the transition? At least in my browser, the border width effect feels a little janky when switching between styles.

[jasmussen]

Agree, let's remove that one. Though the idea of "transitioning between styles" is interesting, I think that will get old quickly, and it likely won't scale to other block transformations either.