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 on slash: /+SEARCH #2284

Closed
wants to merge 1 commit into from
Closed

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Aug 8, 2017

No description provided.

@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Aug 8, 2017
@ellatrix ellatrix self-assigned this Aug 8, 2017
@ellatrix ellatrix force-pushed the try/slash-command branch from 9e10abf to 7cc8328 Compare August 8, 2017 12:58
@ellatrix
Copy link
Member Author

ellatrix commented Aug 8, 2017

Something is still going wrong with the CSS here. Maybe because I'm cross-importing and it causes this?

I wonder how I should improve this? Move InserterMenu to components? But then it still needs to be state aware? Make a special blocks folder in editor/ that is state aware (internal)? Note that this is not the only block, there will be blocks like table of contents that will also need to be state aware. Cc @youknowriad @aduth.

@codecov
Copy link

codecov bot commented Aug 8, 2017

Codecov Report

Merging #2284 into master will increase coverage by 1.08%.
The diff coverage is 17.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2284      +/-   ##
==========================================
+ Coverage   24.06%   25.14%   +1.08%     
==========================================
  Files         149      150       +1     
  Lines        4592     4955     +363     
  Branches      775      876     +101     
==========================================
+ Hits         1105     1246     +141     
- Misses       2946     3109     +163     
- Partials      541      600      +59
Impacted Files Coverage Δ
editor/selectors.js 95.51% <ø> (-0.95%) ⬇️
blocks/editable/patterns.js 1.6% <0%> (-0.22%) ⬇️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
blocks/library/inserter/index.js 20% <20%> (ø)
editor/inserter/menu.js 45.75% <80%> (-2.66%) ⬇️
blocks/library/paragraph/index.js 43.33% <0%> (-3.73%) ⬇️
editor/actions.js 35.29% <0%> (-2.64%) ⬇️
editor/block-settings-menu/index.js 0% <0%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee1083a...7cc8328. Read the comment docs.

@mtias
Copy link
Member

mtias commented Aug 16, 2017

@iseulde I think we should do a separate inserter component for this case—that just lists blocks in a single column list—instead of reusing the other.

@mtias mtias added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Aug 16, 2017
@mtias
Copy link
Member

mtias commented Aug 16, 2017

Probably call it blocks-autcomplete. And it would look closer to:

image

@mtias
Copy link
Member

mtias commented Aug 16, 2017

We should use the same design for eventually autocompleting tags, users, etc.

@ellatrix
Copy link
Member Author

@mtias Ok, I'll adjust

@jasmussen
Copy link
Contributor

Love this, thank you for working on it.

I would agree with Matías, for this round 2 of the slash command, let's try adhering as closely as possible to the pattern from whence the inspiration came: Slack and IRC:

slashcommand

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.

I wonder how I should improve this? Move InserterMenu to components? But then it still needs to be state aware? Make a special blocks folder in editor/ that is state aware (internal)? Note that this is not the only block, there will be blocks like table of contents that will also need to be state aware. Cc @youknowriad @aduth.

I agree, we will likely need better support for blocks components and perhaps blocks themselves having access to and manipulating state. Remarked previously at at #2119 (comment) and #2182 (comment) , I question whether our split between blocks and editor is sensible. I think there are post-editor specific concerns that might not be relevant to blocks and editors generally, but the idea of a block and editor should be unified, at which point the core of an editor consists of block APIs, components, and editor state itself. It becomes the editor chrome (headers, sidebars, etc) which might need to be externalized as a layer atop this core.

}

// Merge text nodes.
editor.getBody().normalize();
Copy link
Member

Choose a reason for hiding this comment

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

How expensive an operation is this for something which will occur on every single keypress? Edit: I guess it's every keypress for a block which implements onReplace (inserter block).

@@ -11,7 +11,7 @@ import { __, _n, sprintf } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { Popover, withFocusReturn, withInstanceId, withSpokenMessages } from '@wordpress/components';
import { keycodes } from '@wordpress/utils';
import { getCategories, getBlockTypes, BlockIcon } from '@wordpress/blocks';
import { getCategories, getBlockTypes, BlockIcon } from '../../blocks';
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?


const { BACKSPACE, ESCAPE } = keycodes;

registerBlockType( 'core/inserter', {
Copy link
Member

Choose a reason for hiding this comment

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

I've not tested this, but what's the expected behavior with this being implemented as a block? A user has to insert the inserter block, then they have the option to / to trigger the menu? I'd not really expected this to be a block, but rather something built-in to either Editable or the default paragraph block.

@aduth
Copy link
Member

aduth commented Sep 20, 2017

Superseded by #2630

@aduth aduth closed this Sep 20, 2017
@aduth aduth deleted the try/slash-command branch September 20, 2017 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants