Inserter: Trying v1 for the block inserter #307

youknowriad · 2017-03-23T10:21:23Z

In this PR, I'm trying to add the block inserter to the plugin. This is the first UI element we make, thus, it raises a lot of questions we need to answer before merging this.

Block API:
In this PR, I'm adding a title and an icon as strings to the block API, we probably need a category as well (I left it for another PR)

React Component
We're relying on createElement which is an abstraction of React to compose our UI. But we'll probably need more than that. Think refs, state ..... Should we export Component as well in wp.element?

In the first iteration of this PR, I avoided explicitly Component and simulated setState on the Editor class.

React Dependencies: react-tooltip, react-clickoutside, ...
Another thing to consider while building the UI is the need of some ready to use React Components (external dependencies like react-tooltip or anything else). I think, even if using these external components would work like a charm, "logically", we can not use them in our UI because we're hiding React, this means we don't know if they're compatible with our abstraction.

JSX
I avoided explicitly JSX here, IMO this is not a big "loss" but this point should be raised I think.

Icons
I'm using the regular WordPress Dashicons (using classnames) in this PR, but I know these are not perfect and we may prefer using SVG. Maybe this could be changed later.

Class properties
I've used .bind (this makes me think I should have moved the call to the constructor) while using https://babeljs.io/docs/plugins/transform-class-properties/ is nicer. Thoughts on adding this babel plugin?

mtias · 2017-03-23T11:54:06Z

modules/editor/inserter/style.scss

+	padding: 0;
+	margin-top: 20px;
+
+	a {


Would be good to start using classes for these.

Would be good to start using classes for these.

And if we do add one, move styles out of nesting. ( Edit: Saw @mtias left a similar comment below, but I'll not remove this so as to reinforce similar reaction 😉 )

mtias · 2017-03-23T11:55:05Z

modules/editor/inserter/style.scss

+	bottom: 40px;
+	background: #fff;
+
+	.inserter__arrow {


Also let's avoid nesting as much as possible when we have a class to keep specificity at the minimum.

mtias · 2017-03-23T11:55:24Z

modules/editor/inserter/index.js

@@ -0,0 +1,22 @@
+const h = wp.element.createElement;


h because this syntax is inspired by 'hyperscript' https://github.com/hyperhype/hyperscript

Right, but I don't think it makes a lot of sense in our context—why not element or node?

I don't know, I feel it should be as short as possible, could we use e?

I have a personal distaste for single letter vars. Though I could compromise on el ;)

I'd prefer something like el as well, but worth noting that h was turned into something of a convention with it also being adopted by preact.

e is typically used for events.

mtias · 2017-03-23T11:56:04Z

modules/editor/blocks/text-block/index.js

@@ -1,4 +1,6 @@
 wp.blocks.registerBlock( 'wp/text', {
+	title: 'Text',


We'll need to start considering i18n in a separate issue.

mtias · 2017-03-23T11:57:24Z

I'm ok with avoiding JSX for now.

aduth · 2017-03-23T13:13:05Z

I'm ok with avoiding JSX for now.

Probably fine for now, but I don't think we ought to go out of our way to avoid it. It will be very easy to integrate and will improve readability.

aduth · 2017-03-23T13:15:34Z

modules/editor/inserter/index.js

@@ -0,0 +1,22 @@
+const h = wp.element.createElement;


I'd prefer something like el as well, but worth noting that h was turned into something of a convention with it also being adopted by preact.

aduth · 2017-03-23T13:16:10Z

modules/editor/inserter/style.scss

+	padding: 0;
+	margin-top: 20px;
+
+	a {


Would be good to start using classes for these.

And if we do add one, move styles out of nesting. ( Edit: Saw @mtias left a similar comment below, but I'll not remove this so as to reinforce similar reaction 😉 )

aduth · 2017-03-23T13:18:49Z

modules/editor/editor.js

@@ -1,8 +1,45 @@
+import InserterButton from './inserter/button';


Should we keep with External, Internal docblocks?

aduth · 2017-03-23T13:19:48Z

modules/editor/editor.js

+
+const h = wp.element.createElement;
+
+const EditorComponent = ( { content, state: { inserter }, toggleInserter } ) => {


Something bothers me about components not being in their own standalone file. Maybe I'm overthinking it. Alternative might be editor/index.js (raw class) and editor/editor.js (component) but then the path becomes editor/editor/editor.js 😬

Also destructuring any more than one level deep quickly becomes unreadable. I think this one isn't so bad but we should be cautious of it.

aduth · 2017-03-23T13:21:31Z

modules/editor/editor.js

+const h = wp.element.createElement;
+
+const EditorComponent = ( { content, state: { inserter }, toggleInserter } ) => {
+	return h( 'div', {},


There might be a minuscule performance benefit to passing null instead of {} because it can be skipped more easily by a simple truthy test.

aduth · 2017-03-23T13:31:04Z

modules/editor/inserter/button.js

+const InserterButton = ( { opened, onClick } ) => {
+	return h( 'div', { className: 'inserter__button' },
+		h( 'a', { onClick },
+			h( 'span', { className: 'dashicons dashicons-plus' } )


Do you think we ought to create a component for Dashicons, assuming this will be a very common pattern? Or is it obvious enough to apply the classes? Where would said component live if we were to introduce it?

There's been a few comments about icons that I'll defer to @jasmussen as to how to proceed.

aduth · 2017-03-24T00:02:07Z

I'm seeing the following error when running npm run dev or npm run build:

ERROR in Entry module not found: Error: Can't resolve './modules/modules/index.js' in '/Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg'

aduth · 2017-03-24T00:09:30Z

The above issue looks to have been caused by lingering files in the modules directory. In this case there was an extra modules folder including only build files, hence why it wasn't apparent in git diff (ignored). Removing it fixed the issue, though I've also seen this in the past with .DS_Store hidden macOS files. I'm guessing our Webpack modules folder globbing is being a little too permissive. We might need to think of a better long-term plan, maybe directly picking modules.

In any case, this needs a rebase, but nothing else stands out to me needing changes. 👍

mimo84 · 2017-03-24T01:23:51Z

modules/editor/assets/stylesheets/main.scss

 .gutenberg {
 	background: #fff;
 	margin: -20px 0 0 -20px;
 	padding: 60px;
+
+	* {
+		box-sizing: border-box;


In our prototype @intronic and me have been using CSS modules which allows for a better style encapsulation and maintainability. I think it's a step above BEM and it would be worth trying. A first step towards this direction would be to start migrating some sections such this one.

I agree CSS modules is really nice. Maybe, we could consider it once we settle the other moving parts.

I'm not too keen on CSS modules (nor it's evolution in styled-components), I think they erode semantics, and complicate things in which CSS is good at (passing props to style things couples the whole node tree with the intentions of a grand-parent that may want to modify a grand-children style props).

In our case I think it makes even less sense because blocks will be restyled by themes, so we do need the strong semantics and ability to externally modify styles. The pseudo BEM here is not strictly BEM either (we don't rely on the same kind of modifier) and is something we've been using in Calypso to reflect the component structure in place.

- Rename h to el - Extract the editor component to its own file

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 23, 2017

youknowriad self-assigned this Mar 23, 2017

youknowriad requested review from mtias and aduth March 23, 2017 10:21

mtias reviewed Mar 23, 2017

View reviewed changes

mtias added the [Type] Task Issues or PRs that have been broken down into an individual action to take label Mar 23, 2017

mtias changed the title ~~Plugin: Trying adding a v1 for the block inserter~~ Inserter: Trying v1 for the block inserter Mar 23, 2017

aduth reviewed Mar 23, 2017

View reviewed changes

mimo84 reviewed Mar 24, 2017

View reviewed changes

aduth mentioned this pull request Mar 24, 2017

Explicitly target entry modules #321

Merged

youknowriad added 2 commits March 24, 2017 10:10

Plugin: Trying adding a v1 for the block inserter

de1315e

- Avoid style nesting

f24298b

- Rename h to el - Extract the editor component to its own file

youknowriad force-pushed the try/adding-inserter branch from a085665 to f24298b Compare March 24, 2017 09:26

youknowriad merged commit 03ec179 into master Mar 24, 2017

youknowriad deleted the try/adding-inserter branch March 24, 2017 09:32

mtias mentioned this pull request Mar 24, 2017

Build: Block Inserter #323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inserter: Trying v1 for the block inserter #307

Inserter: Trying v1 for the block inserter #307

youknowriad commented Mar 23, 2017 •

edited

Loading

mtias Mar 23, 2017

aduth Mar 23, 2017

mtias Mar 23, 2017

mtias Mar 23, 2017

youknowriad Mar 23, 2017

mtias Mar 23, 2017

youknowriad Mar 23, 2017

mtias Mar 23, 2017

aduth Mar 23, 2017

joyously Mar 23, 2017

mtias Mar 23, 2017

mtias commented Mar 23, 2017

aduth commented Mar 23, 2017 •

edited

Loading

aduth Mar 23, 2017

aduth Mar 23, 2017

aduth Mar 23, 2017

aduth Mar 23, 2017

aduth Mar 23, 2017

aduth Mar 23, 2017

aduth Mar 23, 2017

mtias Mar 23, 2017

aduth commented Mar 24, 2017

aduth commented Mar 24, 2017

mimo84 Mar 24, 2017

youknowriad Mar 24, 2017

mtias Mar 24, 2017

		@@ -1,4 +1,6 @@
		wp.blocks.registerBlock( 'wp/text', {
		title: 'Text',

		@@ -1,8 +1,45 @@
		import InserterButton from './inserter/button';


		const h = wp.element.createElement;

		const EditorComponent = ( { content, state: { inserter }, toggleInserter } ) => {

Inserter: Trying v1 for the block inserter #307

Inserter: Trying v1 for the block inserter #307

Conversation

youknowriad commented Mar 23, 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

Choose a reason for hiding this comment

mtias commented Mar 23, 2017

aduth commented Mar 23, 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

aduth commented Mar 24, 2017

aduth commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Mar 23, 2017 •

edited

Loading

aduth commented Mar 23, 2017 •

edited

Loading