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

Show inserter next to empty text block #565

Closed
wants to merge 2 commits into from
Closed

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 28, 2017

Related: #323

This pull request seeks to display an inserter next to the empty text block.

Implementation notes:

  • How do we consider a block as "empty" ? Compare against its default attributes? Expect as undefined? Do a deep traversal to determine if there's any renderable content?
  • How do we allow a block to trigger an inserter to be shown, without forcing it to become too aware of internal components (arguably, a block should not have awareness of an Inserter component)
    • Make editor aware of text block and its empty shape, rendering inserter from VisualEditorBlock directly
  • There can be multiple block movers on screen at the same time (selected and hovered). How do we ensure that only the selected empty block's area shows the inserter. Maybe a separate Fill context needs to be created specific to the selected block, instead of trying to fit this in with the Mover area?
    • Make Inserter a sibling, not child of, BlockMover

@aduth aduth added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Apr 28, 2017
@ellatrix
Copy link
Member

Do we need to show it to any blocks other than the core text block?

@aduth
Copy link
Member Author

aduth commented May 1, 2017

Do we need to show it to any blocks other than the core text block?

Based on discussion following #323 (comment) , the consensus seems to be that this between-content inserter should be shown only next to an empty text block, and more generally that an empty text block is the default empty block when creating new content via the Enter key.

@jasmussen
Copy link
Contributor

Ran into an issue today that I thought I'd mention here:

If you insert a text block (any text, including heading, list, etc.), it scrolls to the inserted block. Good! If you insert a block that doesn't have Editable, like embed or image, it doesn't scroll to the block. Let me know if I should ticket this separately.

@aduth
Copy link
Member Author

aduth commented May 9, 2017

Let me know if I should ticket this separately.

Yes, let's create a separate issue for this. It might be that TinyMCE is scrolling the page when it initializes, in which case we should try to disable that default and implement a consistent approach across all blocks.

@ellatrix
Copy link
Member

ellatrix commented May 9, 2017

@aduth
Copy link
Member Author

aduth commented May 9, 2017

Sharp eye! That sounds perfect.

@aduth aduth force-pushed the add/empty-block-inserter branch from 9728eeb to 40878cf Compare May 15, 2017 19:46
@aduth aduth changed the title (WIP) Show inserter next to empty text block Show inserter next to empty text block May 15, 2017
@aduth aduth force-pushed the add/empty-block-inserter branch from 40878cf to 1592c27 Compare May 15, 2017 19:51
@aduth
Copy link
Member Author

aduth commented May 15, 2017

This is now ready for review. I've updated the original comment with specific remarks to my concerns, and included a fair bit of commentary to the code itself.

@jasmussen
Copy link
Contributor

I don't know if I'm doing it right, but I can't get the inserter to show. I place the cursor in text, press enter twice to create a new block, but all I see is the block mover on the side.

@youknowriad
Copy link
Contributor

What's the status of this branch?

I tested a bit and it seems to work only if I add the text block from the inserter and not by hitting double enter.

And I noticed some z-index issues

screen shot 2017-05-17 at 14 22 26

// When choosing block from inserter for a new empty block, override
// insert behavior to replace current block instead
const { uid, replaceBlocks } = this.props;
replaceBlocks( [ uid ], [ createBlock( slug ) ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the newly created block borrow the uid from the replaced block to stay selected?

'core/text' === block.blockType &&
! block.attributes.content
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this it will work but I prefer your idea here to compute the isNew flag:

#798 (comment)

@aduth
Copy link
Member Author

aduth commented May 18, 2017

Closing due to revised insertion flows proposed at #833.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants