Skip to content

Conversation

@youknowriad
Copy link
Contributor

In this PR, I'm enabling the possibility to effectively add new blocks using the inserter. In the same time I'm adding the "filtering" logic. You can now filter the inserter by filling the search input.

Also on a technical side:

  • We have a new INSERT_BLOCK action
  • I've simplified the hovered and selected reducers to only store one value (we can only have one selected or one hovered block).

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 12, 2017
@youknowriad youknowriad self-assigned this Apr 12, 2017
@youknowriad youknowriad requested a review from aduth April 12, 2017 09:51
Copy link
Member

@aduth aduth Apr 12, 2017

Choose a reason for hiding this comment

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

I was thinking it might be helpful to move this logic into the block API, like:

wp.blocks.createBlock( 'core/text', {
	content: 'Hello World'
} );

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!! 👍

Copy link
Member

Choose a reason for hiding this comment

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

Total aside: How do you feel about a keywords property for a block that allows it to be discovered by more than just its title? Do we also want to search slug?

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 agree this is too limited, we need to think more about how we search for blocks. Keywords seem like a good idea, we could consider adding a description to the blocks.

Copy link
Member

Choose a reason for hiding this comment

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

Another note is we should probably extract some of these utilities as standalone functions and unit test them. Fine for another pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Should we clear the current filter value?

editor/state.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not here, but we'll need to consider how to insert a block at a specific index, supporting behavior like in this mockup:

Insert at index

https://wpcoredesign.mystagingwebsite.com/gutenberg/

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 think this action can be extended. For example, in #409 I'm adding an option "after" property to this action to insert after a specific block.

@aduth
Copy link
Member

aduth commented Apr 12, 2017

When inserting a new text block, there's an error logged to the console from TinyMCE:

tinymce.min.js?ver=4.7.3:11 Uncaught TypeError: Cannot read property 'length' of undefined

Another oddity which I expect is a consequence of the above error is that it's not possible to unselect the newly added text block.

@youknowriad
Copy link
Contributor Author

youknowriad commented Apr 13, 2017

I addressed the feedback in e943478

we now have a new utility createBlock to create a block given its slug and attributes. The factory file could contain other helpers to create a block (from another block for example etc...)

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Works great 👍 Nice work.

attributes: getBlockAttributes( blockNode, settings )
} );
memo.push(
createBlock( blockType, getBlockAttributes( blockNode, settings ) )
Copy link
Member

Choose a reason for hiding this comment

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

Cool re-use 👍

Copy link
Member

Choose a reason for hiding this comment

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

Another note is we should probably extract some of these utilities as standalone functions and unit test them. Fine for another pull request.

@youknowriad youknowriad merged commit 9864131 into master Apr 13, 2017
@youknowriad youknowriad deleted the add/inserter-logic branch April 13, 2017 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Framework Issues related to broader framework topics, especially as it relates to javascript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants