Inserter: Add a visual placeholder at the position where the block is going to be inserted #835
Conversation
youknowriad
added
[Type] Task
editor/actions.js
Outdated
| }; | ||
| } | ||
|
|
||
| export function setBlockToInsert( slug, after ) { |
There was a problem hiding this comment.
I scratched my head a little bit on what this name implied :)
There was a problem hiding this comment.
Please give me a better name. I know naming in this PR particularly is really bad. Naming this state is not easy :P. I scratched my head to find these names.
There was a problem hiding this comment.
maybe setBlockInsertionPoint or something like that.
There was a problem hiding this comment.
Why do we need an "insert block" and a "set block to insert" action?
There was a problem hiding this comment.
When we hover the inserter block, we need to show the placeholder right after the selected block. I guess we don't really need the exact slug of the block to be inserted, right now, we just need to know that we're going to insert a block but I thought this information could be useful in the state.
There was a problem hiding this comment.
If we are only talking about the temporary blue line, I think we should call that setInsertionPoint
There was a problem hiding this comment.
Let's do that and remove the "slug" completely from the state.
editor/state.js
Outdated
| @@ -279,6 +279,22 @@ export function hoveredBlock( state = null, action ) { | |||
| } | |||
|
|
|||
| /** | |||
| * Reducer returning the block slug to be inserter. | |||
editor/modes/visual-editor/block.js
Outdated
| @@ -229,6 +232,7 @@ export default connect( | |||
| block: getBlock( state, ownProps.uid ), | |||
| isSelected: isBlockSelected( state, ownProps.uid ), | |||
| isHovered: isBlockHovered( state, ownProps.uid ), | |||
| isInsertionPoint: isBlockBeforeInsertPoint( state, ownProps.uid ), | |||
There was a problem hiding this comment.
I believe we should create a new element at the block insertion point, otherwise the insertion point will be determined by the specificities of the block-before-insertion-point. Example: if the block is a wide image, the blue line will be full width.
youknowriad
force-pushed
the
add/insertion-point
branch
from
4865398 to
9b5541a
Compare
|
I love this. Thank you for working on it. Can't wait for the blue line to also work after the block that's currently focused. |
|
@jasmussen I rebased since the other ticket got merged so it should already work |
|
Nice, yes, works now! Does the element exist in the flow of the dom or is it overlaid? If the former, it'd be nice if the line itself actually got a little margin. It's okay that it has to push things around a bit. Is this possible or very difficult? |
It's an element that exits in the dom. I think it shouldn't push things around though (It's absolute positioned to avoid this), because if you use the inserter at the bottom of the editor, you'll notice a jump in the inserter when you hover a block to insert. |
|
👍 👍 I can work with that. Can we push the blue line down a bit though? Like another 4 or 6 pixels (or whatever it takes)? Currently looks like this:
Can it look more like this?
|
|
If there's no objection I'm keen to merge this soon |
|
If there are no code concerns, visually it's in a good place! 👍 |
| undefined, | ||
| ( state ) => { | ||
| return { | ||
| selectedBlock: getSelectedBlock( state ), |
There was a problem hiding this comment.
It seems to me that the inserter should have no knowledge of what block is selected. The inserter, in a way, only cares about inserting a block and dispatching the "show insertion point". Once we process the insert action the app would know the selected block internally and handle that aspect. What are your thoughts there?
There was a problem hiding this comment.
Are you suggesting we trigger the insertBlock action here without specifying the position where we'll insert the block (see after argument in onInsertBlock)?
I guess this is possible, but this complexifies the blocks.order reducer a lot, because we'll need to be aware of the insertedBlock sub-reducer value inside the blocks.order reducer to find the right position. Which means we can't use combineReducers utility.
I think we shouldn't introduce this complexity in the state and instead have all the required information to process the insert in the INSERT action.
… going to be inserted
youknowriad
force-pushed
the
add/insertion-point
branch
from
660bd11 to
c1fed33
Compare
|
Merging, I'll address any concern separately. |
|
If I'm not wrong, this PR removed also the hover/focus style on the inserter menu items that used |
I will open a separate ticket with designs for hover and focus styles for our various UI components. then we can apply this across the UI from a single source of best practices. |
|
@afercia I made this change because it's explicitly shown as is the mockups of the issue. Let's fix this separately though |
|
Sure. Maybe the screenshots were just a bit outdated/unrelated. Thanks. |
closes #833
This is not completely functional, because each time we click the inserter, the block is unselected which is not the desired behaviour and should be fixed by #827Anyway, this is adding the placeholder at the end of the editor right now.