Inserter - basic support for block addition #88

mzorz · 2018-08-01T14:05:51Z

This PR has the minimum needed in order to make new blocks appear in the list.

tap on a non-editable bock to gain focus and make the item toolbar appear
tap on the + icon
scroll down the list and observe a new block has been added.
repeat as many times as you wish, and observe you can also delete blocks.

For now, we're producing only one type of block paragraph (even when the function is parameterized) and only appending to the list (I have plans to make direct insertion). Also, we make use of the current item toolbar, as I plan to make small steps / PRs as we go to make small additions (like a standalone + that checks the currently focused item and appends a new block right below, and so on).

The list of steps is available here which I'll use as a master issue and link this and upcoming PRs there as well.

I plan to make all PRs against the feature/inserter, and merge that one to master once all done.

Note (and question at the same time) for the reviewers: in order to make this work, I'm creating an empty block object in two places: one for the list as in the UI element, and another one in the reducer as in the Redux model. It bugs me that I'm creating 2 different objects, so I'm most probably not doing something right. While I'm exploring it, please don't hesitate to provide any insights as to whether this should be fine or not, and if not, feel free to provide any alternatives you can think of to make this cleaner.

Known issue: the item toolbar does not appear when the new items are focused (tapped on). Working on that for a bit, but I don't think is something I should spend much time on, as the toolbar will be handled differently anyway.

Known issue: #88 (comment)

…dataSource

mzorz · 2018-08-01T15:01:49Z

Known issue: the item toolbar does not appear when the new items are focused (tapped on). Working on that for a bit, but I don't think is something I should spend much time on, as the toolbar will be handled differently anyway.

Actually looking closer I think this is quite tight related to the other observation (having 2 different objects is no good). Idea: I think we should rather create in one place (the redux store) and then request a re-render as the list has changed, but not change the in-memory list (it should change itself by following the cycle)

mzorz · 2018-08-01T16:55:42Z

Fixed the duplicate object / focus problems by correctly implementing createBlock in 2beaebd

…Flow errors

…k interface

mzorz · 2018-08-02T20:20:59Z

After reviewing this one but before merging it, please review and merge #89 first 👍

…block-gb Inserter - use GB createBlock to generate new blocks

…sed: false prop in new object

mzorz · 2018-08-08T16:58:40Z

This PR has a problem keeping up with the focused state on newly created blocks, which is addressed in #95 (targeting this branch). That PR should be merged first 👍

hypest

I left a couple of comments @mzorz , please have a look.

All in all though, happy to see this first PR for the Inserter! Also, thanks for updating some tests in here!

hypest · 2018-08-10T15:33:43Z

src/block-management/block-manager.js


 export type BlockListType = {
 	onChange: ( clientId: string, attributes: mixed ) => void,
 	focusBlockAction: string => mixed,
 	moveBlockUpAction: string => mixed,
 	moveBlockDownAction: string => mixed,
 	deleteBlockAction: string => mixed,
+	createBlockAction: ( string, BlockType ) => mixed,


Hmm, apparently we're passing an already instantiated block to the action so, I wonder if the naming ("createBlock...") is a bit off here. I'd kinda expect to pass some identifier and/or params in order for the reducer to instantiate the block. Not sure which way is the web side of Gutenberg's doing this.

Rightly spotted @hypest - as discussed on Slack, this is something I had plans for in a later PR, to take care of bringing this closer to GB 👍
Trackerd in my item list here #58 (comment)

hypest · 2018-08-10T15:39:46Z

src/store/actions/test.js

@@ -32,5 +35,13 @@ describe( 'Store', () => {
 			expect( action.type ).toEqual( ActionTypes.BLOCK.DELETE );
 			expect( action.clientId ).toEqual( '1' );
 		} );
+
+		it( 'should create an action to create a block', () => {
+			registerCoreBlocks();


This feels like a big thing to do while in a section of an otherwise simple test where we just test the fields of a newly created action object.

Can we perhaps mock the block to pass it to the action creation function?

On the other hand, judging from the convo we had over Slack, this will probably change when the block creation itself might move into the reducer so, perhaps we can leave this PR as is.

That's a good observation - also this test for now tests for creation action type only but I think should check for each of the registered blocks if that makes sense (once the function actually creates a block instead of just carrying a block to be added/inserted).

Added to my item list #58 (comment)

I'm still not comfortable with registerCoreBlocks being inside the testcase since it's not the subject of the test itself. Let's move it to the beforeAll section so it's clear that it's part of the setup, not the test. WDYT?

addressed in a04c5e9

hypest · 2018-08-10T15:40:16Z

src/store/actions/test.js

+			const newBlock = createBlock( 'core/code', { content: 'new test text for a core/code block' } );
+			const action = actions.createBlockAction( '1', newBlock );
+			expect( action.type ).toEqual( ActionTypes.BLOCK.CREATE );
+		} );


Any particular reason for not testing the clientId value too as in the rest if this test case?

addressed in 6ad3813 - and for completeness added check for the block in e556378

hypest · 2018-08-13T11:38:56Z

src/store/actions/test.js

@@ -2,6 +2,9 @@

 import * as actions from './';
 import ActionTypes from './ActionTypes';
+// Gutenberg imports
+import { getBlockType, serialize, createBlock } from '@wordpress/blocks';


getBlockType and serialize don't seem to be used anymore, can we remove them?

nice catch! addressed in 54220f8

Thanks for the change @mzorz ! Apparently, the eslint configuration is actually faulty at the moment since this one should be caught during yarn lint (which is already part of the Travis tests).

I will produce a separate PR with the eslint fix (and also fix there a few other violations) so to not pollute this PR, cool?

Thanks for explaining and describing where things are / need to be and follow up actions🙇 thank you! Also think the decision to make a separate PR for eslint fixes is in the right direction 👍

hypest

Looks good to me @mzorz !

mzorz added 11 commits July 27, 2018 18:17

added createBlockAction and CREATE action type

fd47cc0

updated dispatcher mapping for createBlockAction

35d3e1c

added reducer for ActionTypes.BLOCK.CREATE actionType

109e3be

changed copypaste wording

f98d844

added ToolbarButton.PLUS button handler inserting new object in List …

6fcd938

…dataSource

extracted block placeholder creation to buildEmptyBlock function

00aa9d3

moved emptyBlock helper function to block-builder within /store

09102c1

Merge branch 'master' into try/inserter-take1

51981d0

removed debugger statements

de92b5e

using ints for new block ids

abd68f4

modified comment

82c3e7f

mzorz requested review from daniloercoli and hypest August 1, 2018 14:05

mzorz mentioned this pull request Aug 1, 2018

Inserter #58

Closed

4 tasks

fixed createBlock action to correctly propagate the change

2beaebd

mzorz added 7 commits August 1, 2018 13:59

updated test

c6ad773

removed logger call

c95ea0b

fix importing type as a value

2abdc07

modified signature of createBlockAction( newId, newBlock ) and fixed …

7dc1b42

…Flow errors

fixed flow and lint errors

f4d6e85

fixed test for block creation

728ef6a

ditch mocked block-builder in favor of using the actual GB createBloc…

57698ed

…k interface

mzorz and others added 5 commits August 2, 2018 17:28

fixed tests

4dec244

removed comment

bf1dd7f

Merge pull request #89 from wordpress-mobile/try/inserter-use-create-…

5e392d5

…block-gb Inserter - use GB createBlock to generate new blocks

Merge branch 'master' into try/inserter-take1

851d191

using new supported core/paragraph block in inserter, and adding focu…

d2d5247

…sed: false prop in new object

mzorz added 3 commits August 3, 2018 11:25

added missing semicolon

f34612b

Merge branch 'feature/inserter' into try/inserter-take1

303f74f

udpated to latest feature base branch

6abc253

mzorz mentioned this pull request Aug 8, 2018

Inserter - fix block manager state handling #95

Closed

mzorz added 3 commits August 10, 2018 10:38

cascading latest changes from master

c3e8f11

updated usage of uid in favor of clientId

9b97ffa

delete added file by mistake

d03931c

hypest reviewed Aug 10, 2018

View reviewed changes

mzorz added 2 commits August 10, 2018 16:14

added clientId check in test

6ad3813

added block check in test

e556378

mzorz mentioned this pull request Aug 11, 2018

Parse html into known blocks #100

Merged

hypest reviewed Aug 13, 2018

View reviewed changes

mzorz added 2 commits August 13, 2018 08:47

tests: moved registerCoreBlock() to beforeAll()

a04c5e9

removed unused imports

54220f8

hypest approved these changes Aug 13, 2018

View reviewed changes

mzorz merged commit 901e5f0 into feature/inserter Aug 13, 2018

mzorz deleted the try/inserter-take1 branch August 13, 2018 12:35

hypest mentioned this pull request Aug 13, 2018

Fix eslint missing src subdirs #107

Merged

This was referenced Aug 14, 2018

Inserter - fix focus handling, awesomely spotted by @hypest #108

Merged

Feature: Inserter - 1st iteration with Picker #123

Merged

mzorz mentioned this pull request Nov 22, 2018

Make createBlockAction create a block instead of carrying a model #271

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inserter - basic support for block addition #88

Inserter - basic support for block addition #88

mzorz commented Aug 1, 2018 •

edited by hypest

Loading

mzorz commented Aug 1, 2018

mzorz commented Aug 1, 2018

mzorz commented Aug 2, 2018

mzorz commented Aug 8, 2018

hypest left a comment

hypest Aug 10, 2018

mzorz Aug 10, 2018

hypest Aug 10, 2018

mzorz Aug 10, 2018 •

edited

Loading

hypest Aug 13, 2018 •

edited

Loading

mzorz Aug 13, 2018

mzorz Aug 13, 2018

hypest Aug 10, 2018

mzorz Aug 10, 2018

hypest Aug 13, 2018

mzorz Aug 13, 2018

hypest Aug 13, 2018

mzorz Aug 13, 2018

hypest left a comment

Inserter - basic support for block addition #88

Inserter - basic support for block addition #88

Conversation

mzorz commented Aug 1, 2018 • edited by hypest Loading

mzorz commented Aug 1, 2018

mzorz commented Aug 1, 2018

mzorz commented Aug 2, 2018

mzorz commented Aug 8, 2018

hypest 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

mzorz Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

hypest Aug 13, 2018 • 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

hypest left a comment

Choose a reason for hiding this comment

mzorz commented Aug 1, 2018 •

edited by hypest

Loading

mzorz Aug 10, 2018 •

edited

Loading

hypest Aug 13, 2018 •

edited

Loading