Plugin: Quote Block Initial Attempt #327

BE-Webdesign · 2017-03-25T03:22:40Z

Here is an initial attempt at using the current API for a quote block.
I just guessed as to how this should be structured, honestly have no
idea but I tried to mimic the codebase the best I could.

youknowriad

Thanks for helping on the blocks 👍 I left some small notes above :). The main thing we should reconsider here is avoiding creation of Editable components for each block, we should reuse the same Editable component for all blocks to ease creating blocks.

youknowriad · 2017-03-25T10:01:53Z

blocks/index.js

@@ -7,6 +7,7 @@ import * as query from 'hpq';

 export { query };
 export { default as Editable } from './components/editable';
+export { default as EditableQuote } from './components/wp-quote';


I think our idea for now, is to use the Editable component in all the places we need something to be editable. Typically the Editable component could leverage TinyMCE or something. So in the context of the quote for example, you'd use the Editable twice, once for the content and once for the citation. (I think the quote block from the per-block prototype is using this approach, could be an inspiration https://github.com/WordPress/gutenberg/blob/master/docs/tinymce-per-block/src/blocks/quote-block/form.js#L95-L133)

youknowriad · 2017-03-25T10:02:49Z

editor/blocks/quote-block/index.js

+
+	attributes: {
+		value: query( 'blockquote', html( 'p' ) ),
+		citation: query( 'cite,footer', text() )


We should always use footer for the citation IMO. Also, why don't we use html instead of text. The citation could include links, formatting etc...

Just did it for this go around, so the output looked nicer, when the actual component is finalized it should definitely be grabbing the html.

Also on the cite vs footer issue, #311 uses cite, post-content.js uses footer, so I put in both for the time being. We will want to make a decision one way or the other and I believe mtias has already made that decision in #311, so maybe we should only parse cite.

Good point. Let's do cite for everything!

There's a difference. footer is a block element, which should be the wrapper of cite (if any). cite can also be used elsewhere, like in figcaption.

Here's some info: https://w3c.github.io/html/grouping-content.html#the-blockquote-element
There are lots of possibilities to format this.

youknowriad · 2017-03-25T10:04:27Z

editor/blocks/quote-block/index.js

+	},
+
+	save( attributes ) {
+		return wp.element.createElement( 'p', null, attributes.value );


We're not using it yet, but it's the responsibility of the save function to return the correct markup to be stored in postContent. Typically, this should return <blockquote><p>content</p><footer>citation</footer></blockquote>

I figured, I was waiting on feedback before going ahead and changing this.

the responsibility of the save function to return the correct markup to be stored in postContent

Should it use renderToString(element) instead as a return value? Or what is the purpose of returning the react element itself?

I think the current idea is to use renderToString outside the block (in a global maner). Relevant to this discussion #284 (review)
#302 (comment)

Sounds good, so save() will be a way to create a clone of the element basically?

Yep, A clone of the output element (could be different than the edit element shown on the editor). I personally still have a preference for return a string instead of an element, but I'm ok trying this way and see how it feels, we can still reconsider later.

nylen · 2017-03-27T16:10:03Z

Rebased and changed wp/ namespace to core/ (see #332).

BE-Webdesign · 2017-03-28T18:39:56Z

Tried rebasing and force pushing. The fork looks fine but for some reason the PR still thinks two files that were removed still exist which is causing the test suite to fail. Any ideas on how to fix?

Update: I made an error in my code, that is the reason for the tests to fail; oooops.

BE-Webdesign · 2017-03-28T21:32:50Z

This seems to be working pretty well now.

youknowriad · 2017-03-29T15:06:36Z

editor/blocks/quote/index.js

+wp.blocks.registerBlock( 'core/quote', {
+	title: 'Quote',
+	icon: 'format-quote',
+	category: 'quote',


The category should be common. see #323 (comment)

youknowriad · 2017-03-29T15:07:44Z

editor/blocks/quote/index.js

+
+	attributes: {
+		value: query( 'blockquote', html( 'p' ) ),
+		citation: html( 'cite,footer' )


Shouldn't this follow the same markup we use for the save?

Probably, the only problem is that this will not work for our example post-content.js if we change it to just cite. We will want to make a decision on the markup.

I'm not against changing the post-content too. Anyway, this is not blocking for me right now

Do you want to do footer or cite for now?

Going to do footer for now.

youknowriad · 2017-03-29T15:08:19Z

editor/blocks/quote/index.js

+			<blockquote>
+				<Editable
+					value={ combinedValue }
+					onChange={ ( newValue ) => onChange( { newValue } ) } />


I think, this should be onChange( { value: newValue } )

That sounds right.

youknowriad · 2017-03-29T15:08:30Z

editor/blocks/quote/index.js

+				<cite>
+					<Editable
+						value={ citation }
+						onChange={ ( newValue ) => onChange( { newValue } ) } />


I think, this should be onChange( { citation: newValue } )

Yup, good catches all around 👍

I think it is { value: newValue } again? value is a prop for the Editable component, where as citation is a prop for the quote component. onChange() is feeding the Editable component.

the onChange is a callback we need to call to update the block attributes parsed originally by the attributes object above. This second Editable is responsible of editing the citation block attribute, so I think we should call onChange( { citation: newValue } )

Okay, I think I understand what you are saying. I'm still not 100% clear on what the role of onChange() is.

Let me try to explain:

On the parent component rendering the blocks, we keep track of the current state (block list) and each time this state is updated we re-render the blocks by calling the edit method of each block.

When this parent component renders the block (calling edit) it provides an onChange callback, the block can call to update its state (its attributes), so anytime a block attribute needs to be changed, we should call onChange({ [ attributeName ]: attributeValue })

calling the onChange will update the state and triggers rendering the blocks again. This is how the unidirectional data flow of React (and similar) works.

I hope this is clear enough

I think I understand that. I am having trouble on figuring out where onChange() hooks up to in the code. And where the onChange is being passed in.

I see :

{ blocks.map( ( block, index ) => <div key={ index }> { wp.blocks.getBlockSettings( block.blockType ).edit( block.attributes ) } </div> ) }

I do not see where the onChange is passed in, in the code currently. Or is there some magic going on somewhere?

Oh I think you need to rebase :)

Looks like it.

Okay it makes a lot more sense now.

youknowriad · 2017-03-29T15:10:10Z

editor/blocks/quote/index.js

+	category: 'quote',
+
+	attributes: {
+		value: query( 'blockquote', html( 'p' ) ),


Maybe we could write html( 'blockquote p' ) to avoid the concat in the edit method

Sounds good!

Here is an initial attempt at using the current API for a quote block. I just guessed as to how this should be structured, honestly have no idea but I tried to mimic the codebase the best I could.

BE-Webdesign · 2017-03-29T17:01:33Z

@youknowriad

I updated everything. Let me know what you think.

youknowriad

We'll probably need some styling to match the prototype UI, but this is good for now. 🚢 it

BE-Webdesign · 2017-03-29T17:08:29Z

Okay, I'll add the styles real quick.

youknowriad · 2017-03-29T17:17:39Z

@BE-Webdesign The text block lacks styling too, so If you want to keep this for later (maybe together with the text block, it's ok too)

BE-Webdesign · 2017-03-29T17:22:54Z

Too late. I added it already and rebased the latest commits into one.

youknowriad · 2017-03-29T17:26:52Z

editor/blocks/quote/style.scss

@@ -0,0 +1,24 @@
+#editor blockquote {


I think it's preferable to add a class to the quote block (Similar to this https://github.com/WordPress/gutenberg/blob/master/docs/tinymce-per-block/src/blocks/quote-block/form.js#L95). Other blocks may use the blockquote with other styling options.

Okay, sounds good. core-quote?

I'm not the best person for naming things hahaha :) Always had trouble with it. I'll choose editor-blocks-quote to match the folder structure, I guess we should define a convention somewhere

I think matching the slug for the block would be a good place to start. That way the css classes are namespaced like the blocks are.

At some point we'll also need to consider if blocks can define their own default styles when rendered on the front-end of a site.

At some point we'll also need to consider if blocks can define their own default styles when rendered on the front-end of a site.

To make it overridable, the fewer inline styles the better. The editor can't prevent the user from adding inline styles in raw mode, but it shouldn't encourage them in visual mode. The theme and plugins will be supplying the styles for the front-end look, though, so the block itself shouldn't have "default" styles beyond what the browser defines. The current method of defining editor-style.css should work the same way for this, right?

Right, I don't think we'd want to encourage any inline styles, which furthers the need to consider stylesheets distinguished between the editor and front-end versions of a block. While the blocks here are first-party, we should outline and use an approach consistent with what we expect from plugin authors who implement their own blocks (a plugin which implements a slider block will need ability to define editor and front-end baseline styles). Whatever we land on, I agree that theme styles should always take precedence when defined for a block.

To be clear, I think the editor should not define any styles for rendering the content in the blocks.
The theme and plugins can supply styles, because they can also put those on the front end. But the editor can't put any on the real front end, so to get the correct preview, it shouldn't put any in the editor either.
As for "blocks defining their own default styles", a block is either core or added by user code, so how is it "defining its own"?

youknowriad · 2017-03-29T17:28:04Z

editor/blocks/quote/style.scss

@@ -0,0 +1,24 @@
+#editor blockquote {
+	margin: 2em;


margin: 0 ?

I was just copying styles that I found in one of the prototypes. I can change these later got to go for now.

Yep, I guess the other one had a margin in all the blocks. The margin felt a bit weird while testing.

aduth · 2017-03-29T19:03:12Z

editor/blocks/quote/index.js

+	category: 'common',
+
+	attributes: {
+		value: html( 'blockquote > p' ),


A blockquote could contain multiple paragraphs. In my explorer demo I use text: query( 'p', html() ) instead which assigns text as an array of each paragraph's HTML.

https://aduth.github.io/hpq/

Oh my bad, it was my recommendation

Could we join the output directly using hpq?

We will probably want to map the values in value to its of <p> component as well right?

Could we join the output directly using hpq?

Not super cleanly, but possible. Since each matcher is just a function receiving a node, you can create your own pass-through:

attributes: { value: ( node ) => query( 'p', html() )( node ).join() }

Or some compose-y equivalent.

I don't know that it's necessary a big issue to need to deal with this in the render behavior though.

aduth · 2017-03-29T19:04:03Z

editor/blocks/quote/index.js

+		return (
+			<blockquote>
+				{ value }
+				<footer>


We may want to account for the case that citation is empty and avoid including the footer if possible. This was one of the reasons for hpq: to account for cases where the markup isn't always necessarily the same in the generated output (maybe a quote citation, maybe an image caption, but not necessarily).

Yup sounds good! For some reason this slipped my brain, great catch. Does hpq return an empty value if there is nothing matched by the query?

Does hpq return an empty value if there is nothing matched by the query?

Yeah, specifically an undefined value if I recall correctly.

aduth · 2017-03-31T15:25:06Z

As of #335, a block's save and edit will receive an object akin to a React component's props where attributes and onChange are keys. The thinking being that the value of save and edit are in fact treated as components. In most usage we'll prefer the stateless function component, but you could even assign:

edit: class extends wp.element.Component {
	componentDidMount() {
		fetch( '/resource' ).then( /* ... */ );
	}

	render() {
		const { attributes } = this.props;
		// ...
	}
}

Not sure yet what naming conventions we'll want to recommend for the argument passed to the function (for ES5 implementations; ES6 can easily destructure). Maybe props is clear enough? Else block.

Just now realizing the docs example needs to be updated.

BE-Webdesign · 2017-04-15T02:16:17Z

Closing in favor of #419.

youknowriad reviewed Mar 25, 2017

View reviewed changes

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

nylen force-pushed the quote-block branch from dcbd374 to 729470d Compare March 27, 2017 16:09

BE-Webdesign force-pushed the quote-block branch 2 times, most recently from 0978081 to 7817999 Compare March 28, 2017 18:17

youknowriad reviewed Mar 29, 2017

View reviewed changes

BE-Webdesign added 5 commits March 29, 2017 12:17

Plugin: Quote Block Initial Attempt

6afa34a

Here is an initial attempt at using the current API for a quote block. I just guessed as to how this should be structured, honestly have no idea but I tried to mimic the codebase the best I could.

Updating quote block to use JSX and Editable component.

45414b6

Fix for tests.

ca95a8c

Fix component to work with new Editable component.

d253927

Updating Quote Block.

89c8dce

BE-Webdesign force-pushed the quote-block branch from e351ec7 to 89c8dce Compare March 29, 2017 16:48

youknowriad approved these changes Mar 29, 2017

View reviewed changes

Updating Quote Block.

3cb0c08

youknowriad reviewed Mar 29, 2017

View reviewed changes

aduth reviewed Mar 29, 2017

View reviewed changes

This was referenced Mar 31, 2017

Plugin: Add Heading Block #337

Closed

Early List block implementation (no UI yet) #358

Merged

Update block API docs with edit/save argument as props #363

Merged

youknowriad mentioned this pull request Apr 13, 2017

Blocks: Adding the quote block #419

Merged

BE-Webdesign closed this Apr 15, 2017

snyk-bot mentioned this pull request Apr 1, 2023

[Snyk] Upgrade markdownlint-cli from 0.31.1 to 0.33.0 naiba4/gutenberg#4

Open

Plugin: Quote Block Initial Attempt #327

Plugin: Quote Block Initial Attempt #327

Conversation

BE-Webdesign commented Mar 25, 2017

youknowriad 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

BE-Webdesign Mar 25, 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

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

nylen commented Mar 27, 2017

BE-Webdesign commented Mar 28, 2017 • edited Loading

BE-Webdesign commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign Mar 29, 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

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

BE-Webdesign Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Mar 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BE-Webdesign commented Mar 29, 2017

youknowriad left a comment

Choose a reason for hiding this comment

BE-Webdesign commented Mar 29, 2017

youknowriad commented Mar 29, 2017

BE-Webdesign commented Mar 29, 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

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

BE-Webdesign Mar 29, 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

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

aduth commented Mar 31, 2017

BE-Webdesign commented Apr 15, 2017

BE-Webdesign Mar 25, 2017 •

edited

Loading

BE-Webdesign commented Mar 28, 2017 •

edited

Loading

BE-Webdesign Mar 29, 2017 •

edited

Loading

BE-Webdesign Mar 29, 2017 •

edited

Loading

youknowriad Mar 29, 2017 •

edited

Loading

BE-Webdesign Mar 29, 2017 •

edited

Loading