Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Early List block implementation (no UI yet) #358

Merged
merged 17 commits into from
Apr 7, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions blocks/components/editable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,7 @@ export default class Editable extends wp.element.Component {
);
}
}

Editable.defaultProps = {
nodeName: 'div'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
8 changes: 8 additions & 0 deletions editor/assets/stylesheets/main.scss
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ body.toplevel_page_gutenberg {
svg {
fill: currentColor;
}

ul {
list-style-type: disc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

ol {
list-style-type: decimal;
}
}

.gutenberg__editor {
Expand Down
2 changes: 2 additions & 0 deletions editor/blocks/index.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
import './freeform';
import './text';
import './list';

43 changes: 43 additions & 0 deletions editor/blocks/list/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
const Editable = wp.blocks.Editable;
const { html, prop } = wp.blocks.query;

function List( { nodeName, children } ) {
// nodeName.toLowerCase() is used to map DOM nodeName values to proper tag.
return wp.element.createElement( nodeName.toLowerCase(), null, children );
}

wp.blocks.registerBlock( 'core/list', {
title: 'List',
icon: 'editor-ul',
category: 'common',

attributes: {
listType: prop( 'ol,ul', 'nodeName' ),
items: wp.blocks.query.query(
'li',
{
value: html()
}
)
},

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const value = items.map( i => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return `<li>${ i.value }</li>`;
} ).join( '' );

return (
<Editable
nodeName={ listType }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

value={ value }
onChange={ onChange } />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
},

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return <List nodeName={ listType }>{ children }</List>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
} );
8 changes: 4 additions & 4 deletions languages/gutenberg.pot
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also: #377

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

msgid "Freeform"
msgstr ""

#: editor/header/mode-switcher/index.js:31
msgctxt "Name for the Text editor tab (formerly HTML)"
msgid "Text"
msgstr ""

#: editor/blocks/freeform/index.js:4
msgid "Freeform"
msgstr ""

#: editor/header/mode-switcher/index.js:30
msgid "Visual"
msgstr ""
Expand Down
5 changes: 5 additions & 0 deletions post-content.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,14 @@ window._wpGutenbergPost = {
'<p>By shipping early and often you have the unique competitive advantage of hearing from real people what they think of your work, which in best case helps you anticipate market direction, and in worst case gives you a few people rooting for you that you can email when your team pivots to a new idea. Nothing can recreate the crucible of real usage.</p>',
'<!-- /wp:core/text -->',

'<!-- wp:core/list -->',
'<ul><li>Ship early</li><li>Ship often</li><li>Listen to feedback from real people</li><li>Anticipate market direction</li></ul>',
'<!-- /wp:core/list -->',

'<!-- wp:core/embed url:https://www.youtube.com/watch?v=Nl6U7UotA-M -->',
'<iframe width="560" height="315" src="//www.youtube.com/embed/Nl6U7UotA-M" frameborder="0" allowfullscreen></iframe>',
'<!-- /wp:core/embed -->',
].join( '' ),
},
};