Early List block implementation (no UI yet) #358

mimo84 · 2017-03-31T04:33:36Z

Slight change to Editable

The change to Editable that we made was to pass in the tag to use as the editable root for the tinymce instance. This enables inline tinymce mode to run on something that isn't just a div. It also avoids the issues that Edwin highlighted in his PR with forced_root_block.

Heading PR (#337) changes

Due to the change we made to Editable, we changed the heading PR to pass in the heading tag as the root node for the Editable component. It may make sense to do something similar for text blocks as well (i.e. pass in a p tag).

Early List Block (#346)

Using the heading/index.js as a guide, we created a basic list implementation. Using the ol/ul as the editor root, we could see two ways of doing this:

pass the list items to the Editable component as an HTML string (like the heading)

We created a list block to have two attributes: listType and items. ListType is a string (either 'ol' or 'ul') and items is an array of item information. At this point, items only have value. We envisage that you may be able to extend the item object format to allow for nested lists.

e.g.

let items = [{ value: 'item 1' }, { value: 'item 2' }]

When the list blocks gets given these items, it turns them into their equivalent HTML string (e.g. <li>item 1</li><li>item 2</li>), and feeds that into Editable as value. This has the advantage of keeping React components separate from tiny's contents, but the disadvantage of having to manually manage the HTML strings. However, we will probably manage these with tiny commands anyway.

create the list items as react components, and change Editable to be given children as well

The second approach we considered (haven't implemented) is to create the entire list structure as react components, and then pass those list items as children components to the Editable component. This has the advantage of being cleaner when manipulating items, but it has the disadvantage of mixing tinymce content with react's rendering. This will likely lead to issues.

Demo section

In order to effectively test the block editing and saving processes in isolation, we created a test page at demo/demo.html. This can run without the Wordpress environment but still requires webpack to run in the background. Is there a better way that we should be doing this?

youknowriad · 2017-03-31T08:24:30Z

This is a duplicate to #347 (regarding the config of the root node)

youknowriad

Thanks for the PR. I think it'll be better if we can split this PR to smaller PRs for each block. Also, make sure to check the linting errors :)

I tested the two blocks, they behave properly. But let's get #347 merged before merging those blocks.

youknowriad · 2017-03-31T08:26:05Z

demo/demo.html

+  </table>
+</body>
+
+</html>


I think this demo page should be removed, we already have the default post content viewable in the plugin page.

youknowriad · 2017-03-31T08:27:11Z

editor/blocks/heading/index.js

+		const { headingType = 'H2', value } = attributes;
+		return <Heading nodeName={ headingType }>{ value }</Heading>;
+	}
+} );


I'd prefer a separate PR for each block, it eases the discussion.

aduth · 2017-03-31T15:26:35Z

See #327 (comment) for relevant changes to edit and save function signature.

Removed the new demo files; Removed the changes for headings; Fixed linting issues in the new list plugin;

aduth · 2017-04-06T21:41:41Z

blocks/components/editable/index.js

@@ -84,3 +84,7 @@ export default class Editable extends wp.element.Component {
 		);
 	}
 }
+
+Editable.defaultProps = {
+	nodeName: 'div'


There's no nodeName prop for Editable and an effective default is already assigned in the render. We can remove this set of lines.

aduth · 2017-04-06T21:42:00Z

editor/assets/stylesheets/main.scss

@@ -25,6 +25,14 @@ body.toplevel_page_gutenberg {
 	svg {
 		fill: currentColor;
 	}
+
+	ul {
+		list-style-type: disc;


Should these be generic across the entire editor canvas or specific to the blocks we're implementing?

I would keep these generic as these styles can be applied to the generic freeform block as well. In the future these might go away if we decide that the list block can support multiple styles of lists.
Without these styles the default list style type is none in the wp-admin page.

aduth · 2017-04-06T21:43:04Z

editor/blocks/list/index.js

+
+		return (
+			<Editable
+				nodeName={ listType }


Editable accepts a tagName, the difference being that the nodeName here will be capitalized. We should toLowerCase it and pass as tagName.

aduth · 2017-04-06T21:43:46Z

editor/blocks/list/index.js

+
+	edit( { attributes, onChange } ) {
+		const { listType = 'ol', items = [] } = attributes;
+		const value = items.map( i => {


In general I think we should avoid non-descript variable names like i in favor of more verbose and clear alternatives; here, item.

aduth · 2017-04-06T21:45:00Z

editor/blocks/list/index.js

+	},
+
+	edit( { attributes, onChange } ) {
+		const { listType = 'ol', items = [] } = attributes;


When not default, listType will be uppercased (nodeName). Should the default be uppercased as well? See note below about Editable tagName.

aduth · 2017-04-06T21:45:35Z

editor/blocks/list/index.js

+	save( { attributes } ) {
+		const { listType = 'ol', items = [] } = attributes;
+		const children = items.map( ( i, index ) => <li key={ index }>{ i.value }</li> );
+		return <List nodeName={ listType }>{ children }</List>;


Since we're not reusing the List function anywhere except here, might be clearer to simply inline it? Up to you.

aduth · 2017-04-06T21:47:54Z

editor/blocks/list/index.js

+
+	save( { attributes } ) {
+		const { listType = 'ol', items = [] } = attributes;
+		const children = items.map( ( i, index ) => <li key={ index }>{ i.value }</li> );


We'll probably need to dangerouslySetInnerHTML here for HTML in value to not be escaped. I'd like to explore options here to make this less... scary, and more obvious / transparent.

const children = items.map( ( item, index ) => ( <li dangerouslySetInnerHTML={ { __html: item.value } } /> ) );

I'd be curious to know whether key matters at this line since we're not doing any sort of reconciliation, we're simply calling renderToString at this point.

aduth · 2017-04-06T21:48:24Z

languages/gutenberg.pot

@@ -3,15 +3,15 @@ msgstr ""
 "Content-Type: text/plain; charset=utf-8\n"
 "X-Generator: babel-plugin-wp-i18n\n"

-#: editor/blocks/freeform/index.js:4


See also: #377

We could probably skip these changes. I'll hope to tackle this in the next day.

Actually, on the topic of localization, we should localize the block's title.

See text bock as example:

https://github.com/WordPress/gutenberg/blob/f1b8d8d/editor/blocks/text/index.js#L5

aduth · 2017-04-07T00:38:07Z

editor/blocks/list/index.js

+			<Editable
+				tagName={ listType }
+				value={ value }
+				onChange={ onChange } />


We can improve this in a future pull request, but just FYI that this isn't really doing anything. You'll probably want to set this up so that onChange specifically updates the items entries.

ephox-mogran and others added 5 commits March 30, 2017 18:26

text and list blocks

f565b8c

starting with list blocks

f35c593

list-block: Trying a possible approach with lists

3e96e6e

list-block: Saving and editing lists

ecf4e28

list-block: restored original inserter

a6b4e9f

anna-harrison mentioned this pull request Mar 31, 2017

Add Block: List #346

Closed

youknowriad reviewed Mar 31, 2017

View reviewed changes

ellatrix mentioned this pull request Mar 31, 2017

Allow Editable to have variable node name. #347

Merged

jasmussen mentioned this pull request Mar 31, 2017

Idea: Smart Text Block #349

Closed

mimo84 added 6 commits April 3, 2017 09:06

Merge remote-tracking branch 'origin' into list-block

ce33c03

Fixing PR comments:

374fb7b

Removed the new demo files; Removed the changes for headings; Fixed linting issues in the new list plugin;

List icon in block inserter

7c7af5f

Added sample content for list

67ed22d

merge conflict

06c44d7

Fixed linting

6a42a5d

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

Merge branch 'master' into list-block

c8bf285

anna-harrison assigned mimo84 Apr 6, 2017

Merged master

69aa433

aduth requested changes Apr 6, 2017

View reviewed changes

mimo84 added 3 commits April 7, 2017 08:10

Removed nodeName from default properties

68427de

Added changes as per PR comments

b00b4b8

restored pot file as per master

821ac5d

intronic mentioned this pull request Apr 6, 2017

Add/346 list block/ui #382

Closed

Added i18n string for list

e370326

aduth approved these changes Apr 7, 2017

View reviewed changes

mimo84 merged commit 2f84506 into WordPress:master Apr 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Early List block implementation (no UI yet) #358

Early List block implementation (no UI yet) #358

mimo84 commented Mar 31, 2017

youknowriad commented Mar 31, 2017

youknowriad left a comment •

edited

Loading

youknowriad Mar 31, 2017

youknowriad Mar 31, 2017

aduth commented Mar 31, 2017

aduth Apr 6, 2017

aduth Apr 6, 2017

mimo84 Apr 6, 2017

aduth Apr 6, 2017

aduth Apr 6, 2017

aduth Apr 6, 2017

aduth Apr 6, 2017

aduth Apr 6, 2017

aduth Apr 6, 2017

aduth Apr 6, 2017

aduth Apr 7, 2017

Early List block implementation (no UI yet) #358

Early List block implementation (no UI yet) #358

Conversation

mimo84 commented Mar 31, 2017

Slight change to Editable

Heading PR (#337) changes

Early List Block (#346)

Demo section

youknowriad commented Mar 31, 2017

youknowriad left a comment • edited Loading

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

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 •

edited

Loading