Skip to content

Conversation

@mzorz
Copy link
Contributor

@mzorz mzorz commented Aug 14, 2018

Closing #96 in favor of this one, as the base branch of #95 is not needed anymore, and avoid messing with commit history

Adds direct insertion at a specific index in the list state. For the sake of the exercise in the GB mobile application for now we're inserting right below the tapped block.

Before, it would only append new blocks to the list.

Android

insertertake1direct

iOS

insertertake1direct_ios


// TODO: block type picker here instead of hardcoding a core/code block
const newBlock = createBlock( 'core/paragraph', { content: 'new test text for a core/paragraph block' } );
const newBlockWithFocusedState = { ...newBlock, focused: false };
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's not part of the PR's diff but, wdyt about doing a small optimization here by just appending to the object a new field instead of creating a full new object?

We can just do:

newBlock.focused = false;

and use the newBlock instead of construct the newBlockWithFocusedState.

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 call! decided to address that in 39f6500 in #112 given this part of the code gets moved around in there 👍

it( 'should create an action to create a block', () => {
const newBlock = createBlock( 'core/code', { content: 'new test text for a core/code block' } );
const action = actions.createBlockAction( '1', newBlock );
const action = actions.createBlockAction( '1', newBlock, '0' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also test the clientIdAbove while at it, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 8814dc0

// TODO we need to set focused: true and search for the currently focused block and
// set that one to `focused: false`.
blocks.push(action.block);
insertBlock(blocks, action.block, action.clientIdAbove);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, should this insertBlock replace the blocks.push in the previous line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops thank Lord you catched that, addressed c1b51b9

// TODO we need to set focused: true and search for the currently focused block and
// set that one to `focused: false`.
const insertionIndex = findBlockIndex( blocks, clientIdAbove );
if (insertionIndex === blocks.length - 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the check? If it's just for guarding against the end of the array, I think splice takes care of that already so we don't need to. See here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL! addressed in 145e82f

@hypest
Copy link
Contributor

hypest commented Aug 14, 2018

Nice one @mzorz ! I left a few comments, have a look and let me know if something is not clear! Thanks!

@mzorz
Copy link
Contributor Author

mzorz commented Aug 14, 2018

Ready for another round @hypest - thank you! 🙇

@mzorz
Copy link
Contributor Author

mzorz commented Aug 14, 2018

flow has given its OK now with the new CreateActionType @hypest - ready for a third round :)

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

Nice flow fix @mzorz!
LGTM!

@mzorz mzorz merged commit deb3ef5 into feature/inserter Aug 14, 2018
@mzorz mzorz deleted the try/inserter-take1-state-props-direct-insert-take2 branch August 14, 2018 17:16
This was referenced Aug 16, 2018
hypest pushed a commit that referenced this pull request Jan 18, 2019
…oid-to-v1.3.14

Bump Aztec Android to v1.3.14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants