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

Inserter: Try showing a "line" between two blocks on hover to insert a new empty block #5198

Merged
merged 6 commits into from Feb 26, 2018

Conversation

Projects
None yet
8 participants
@youknowriad
Contributor

youknowriad commented Feb 22, 2018

Feedback suggests that people care about the in-between blocks inserter.
This inserter had some issues: being too distractive, some issues related to the big click area.
This PR explores an alternative showing a gray line between two blocks on hover and when you click it, you add a new empty block at that position. At that moment you'll be able to insert any other block there.

kapture 2018-02-22 at 11 21 58

@youknowriad youknowriad self-assigned this Feb 22, 2018

@youknowriad youknowriad requested review from mtias and jasmussen Feb 22, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 22, 2018

Just looking at the GIF, I love this, and glad to see the sibling inserter return.

I think this looks like a good implementation. It may be worth getting this one in quickly, and if it isn't obvious enough, we can fade a plus icon in as well. But given it invokes the in-canvas inserter on a newline (yay for fewer and consistent interfaces), I think this is a good first step to try.

Can the gray separator be moved down 2 pixels, so it's perfectly centered between the blocks?

Can the gray separator have left and right margins, so it's as wide as the content inside the blocks, not the contents + block padding?

With that, 👍 👍

On a personal note — I take full responsibility for removing the sibling inserter as part of the test to see if we could do without it. While I still think it was a valuable test: the results are clear that we can't do without it, given how many eyes are on Gutenberg right now it's definitely sub-optimal to test this in plugin releases, even if we are still in beta. The biggest reason being: unless people read the changelog, they have no good chance to know that it's part of a test and that the UI they relied on might return. It definitely difficult to balance the need to improve UIs and iterate fast, with the need to not alienate testers. Sorry about that, and sorry about the amount of feedback it directed negatively at you specifically. If anything it should've been directed at me.

@hedgefield

This comment has been minimized.

hedgefield commented Feb 22, 2018

Love this. Feels natural to click between blocks, I found myself doing this often when wanting to insert at a specific point.

@hedgefield

This comment has been minimized.

hedgefield commented Feb 22, 2018

And I wouldn't feel too bad about the feedback, this is still a beta, we have to be free to test things out and sometimes radically change something if it will turn out for the better. In this case, we know something we didn't know before testing all the options :)

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 22, 2018

Sorry about that, and sorry about the amount of feedback it directed negatively at you specifically. If anything it should've been directed at me.

Don't worry too much Joen, I don't mind receiving and replying to feedback. And I was fully behind the decision to remove the inserter for consistency.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 22, 2018

Thanks folks, appreciate it. I'm personally okay with receiving a little flak for what in hindsight might have been a little haphazard. I just wish it didn't reflect onto others. Appreciate the support.

Love this. Feels natural to click between blocks, I found myself doing this often when wanting to insert at a specific point.

I agree — it's also mimicking the pattern you'd employ in a non-block editor. In Google Docs if you want to insert an image between two other images, the natural inclination is to click there and make a linebreak.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 22, 2018

Updated with the tweaks. Nothing that if one adjacent block is selected, the line is a bit smaller than the border of the selected block

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 22, 2018

I think it looks great! Forgot to mention — can we fade in the line much faster?

My transition math isn't super great, looking at transition: opacity 0.8s linear 0.5s; does that mean fade in over a .8s period after a .5s wait? If yes, could we try instead transition: opacity 0.2s linear 0.5s;? That is, fade in over a .2s period after a .5s wait?

@afercia

This comment has been minimized.

Contributor

afercia commented Feb 22, 2018

How this is supposed to work when using only the keyboard? I'd like to remind that any functionality should be device-independent and thus work also with keyboard only. In this discussion I see you're considering only hover and click. The button seems to always have tabIndex="-1" so it's not focusable.

At this point of the development I'd like to see features being designed since the beginning with some basic accessibility level. 🙂

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 22, 2018

At this point of the development I'd like to see features being designed since the beginning with some basic accessibility level.

For me this is one of the features that shouldn't be used with keyboard because it goes against tabbing between/in blocks and has alternatives with keyboard. So, with accessibility in mind, I added the tabIndex="-1" to avoid breaking accessibility of the editor. If that's wrong I'd be happy to update it with what you think is best.

@afercia

This comment has been minimized.

Contributor

afercia commented Feb 22, 2018

Keyboard is just one type of device. There are many others. For this reason I've mentioned "device independence". Many alternative devices and software work in sort of "keyboard emulation" mode.
Actually, this is a mouse only feature. Won’t even work with touch.

I'd also like to remind the original sibling inserter (the one with the + icon) perfectly worked with keyboard. Not sure why the argumentations seems to have changed now.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 22, 2018

Not sure why the argumentations seems to have changed now

That's just my argumentation, I didn't build the original sibling inserter so I can't speak for it. I'm just asking for actionable items here. What should I do? do you think I should remove the tabIndex?

@hedgefield

This comment has been minimized.

hedgefield commented Feb 22, 2018

What Andrea is trying to say is that back when the + button was in there, you could focus it with Tab and press Enter to get a new block. Not being able to focus it now is a regression in that sense. So yeah, removing the tabindex would be sufficient, if I understood correctly.

@afercia

This comment has been minimized.

Contributor

afercia commented Feb 22, 2018

@youknowriad I'm not even sure why the original inserter with the + icon was changed :)
Anyways, I'd say the + icon worked from a functional and a11y perspective.
Reintroducing the same sibling inserter with a different look but with an a11y regression doesn't seem ideal to me. If it doesn't break anything, yes I'd like to see this inserter focusable.

@brettsmason

This comment has been minimized.

brettsmason commented Feb 22, 2018

This looks like a big improvement nice job!

My only comment from testing Gutenberg a bunch with clients/friends is a plus icon on hover/select or whatever might be nice in addition so you can directly insert a block rather than first getting the inserter to come up, thus cutting out a click.

Hope that makes sense...

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 22, 2018

My only comment from testing Gutenberg a bunch with clients/friends is a plus icon on hover/select or whatever might be nice in addition so you can directly insert a block rather than first getting the inserter to come up, thus cutting out a click.

We had this, and it was getting in the way of the writing flow. We need to find a balance between using Gutenberg as an editor (writing flow) and using it as a content builder (adding blocks). That's just my opinion but the current solution seems like a good middle ground approach to avoid several ways of inserting content while not making so hard.

@aduth

Looks and works generally well 👍

Couple usage feedback:

  • At the end of content where last block is non-default, should I expect to see both the inserter button and the default block appender?
  • Should we have an inserter before the first block?
'.editor-block-contextual-toolbar': 2,
'.editor-block-switcher__menu': 2,
'.components-popover__close': 2,
'.editor-block-list__block > .editor-block-list__insertion-point':2,

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

Minor: Space after colon.

return null;
}
function BlockInsertionPoint( { showInsertionPoint, showInserter, index, layout, rootUID, ...props } ) {
const onClick = () => {

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

For a component which is rendered for every block, I think we ought to be conscious of the referential equality of functions in render reconciliation and pre-bind these into a component class.

function BlockInsertionPoint( { showInsertionPoint, showInserter, index, layout, rootUID, ...props } ) {
const onClick = () => {
props.insertDefaultBlock( { layout }, rootUID, index );
props.startTyping();

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

Should startTyping be an effect of insertDefaultBlock?

This comment has been minimized.

@youknowriad

youknowriad Feb 23, 2018

Contributor

While we use it this way in general, I wonder if it makes sense conceptually. A third-party plugin would want to trigger insertDefaultBlock programmatically without impact on the writing flow.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 23, 2018

feedback addressed, I'll defer to @jasmussen about the usability feedback from @aduth

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 23, 2018

At the end of content where last block is non-default, should I expect to see both the inserter button and the default block appender?

I would think not. The pattern for this iteration of the in-between/sibling inserter, is to mimic that of making a linebreak. As there already exists sort-of-a linebreak at the end of the post, this one shouldn't be necessary.

Should we have an inserter before the first block?

That would potentially be nice, it seems like? If it feels good in the branch, no reason not to.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 26, 2018

Feedback addressed, waiting for the tests before merging

@youknowriad youknowriad merged commit 1232d37 into master Feb 26, 2018

2 checks passed

codecov/project 39.4% (-0.06%) compared to 2957b51
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/inbetween-inserter branch Feb 26, 2018

@@ -396,6 +430,21 @@
}
}
.editor-block-list__layout > .editor-block-list__insertion-point {
position: relative;
margin-top: -15px;

This comment has been minimized.

@aduth

aduth Feb 26, 2018

Member

This causes misalignment of the first block, overlapping the title and putting the text caret at the wrong position when clicking within the writing prompt:

prompt

The line also extends beyond the correct bounds of the block list / title:

image

This comment has been minimized.

@aduth

aduth Feb 26, 2018

Member

Also noting that the misalignment impacts nested blocks, and should be accounted for:

image

(The tops of the squares should align)

@aduth

This comment has been minimized.

Member

aduth commented Feb 26, 2018

This is now the default focus target for nested blocks (the bug which occurs when pressing menu). The line itself it doesn't look so great:

image

@@ -215,6 +216,7 @@ class BlockListLayout extends Component {
return (
<BlockSelectionClearer className={ classes }>
{ !! blockUIDs.length && <BlockInsertionPoint /> }

This comment has been minimized.

@aduth

aduth Feb 26, 2018

Member

This needed to be passed rootUID. When trying to insert a block before the first block in a Columns block, it appears in the top-level instead.

<div className="editor-block-list__insertion-point">
{ showInsertionPoint && <div className="editor-block-list__insertion-point-indicator" /> }
{ showInserter && (
<button

This comment has been minimized.

@aduth

aduth Feb 26, 2018

Member

Anticipating a future bug: Arrowing up or down from a block will focus the between-inserter.

There's no good reason we don't include button in this set:

getVisibleTabbables() {
return focus.tabbable
.find( this.container )
.filter( ( node ) => (
node.nodeName === 'INPUT' ||
node.nodeName === 'TEXTAREA' ||
node.contentEditable === 'true' ||
node.classList.contains( 'editor-block-list__block-edit' )
) );
}

...Actually, I'm seeing the "good reason", and it's to avoid focus moving into the block toolbar. This is very fragile, as I'd guess if someone did something like add an input (input type="button") into the toolbar, it'd break as-is.

@aduth

This comment has been minimized.

Member

aduth commented Feb 26, 2018

As far as I can tell, this just doesn't work at all for nested blocks. I think it's an issue of layout not being assigned.

@mcsf

This comment has been minimized.

Contributor

mcsf commented Feb 27, 2018

This surfaced #5291.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 27, 2018

It should be fixed with #5282

@aduth

This comment has been minimized.

Member

aduth commented Feb 27, 2018

@youknowriad In testing #5291 while on #5282, the issue still persists. Might be something separate.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 27, 2018

Right I misread the issue :) I'll take a deeper look tomorrow morning

@aduth

This comment has been minimized.

Member

aduth commented Feb 27, 2018

#5291 has proposed resolution in #5296
#5198 (comment) has proposed resolution in #5295

@StaggerLeee

This comment has been minimized.

StaggerLeee commented Mar 2, 2018

I think you should remove block if clicked between blocks and moved away in work. Beginners tend to click around a lot, and it will result in many, really many, empty blocks.

Each click makes new block. Not each click is meant to be a block.

Have some feeling all this is unnecessary made complicated. You are haunt by reducing number of clicks. But sometimes it ends in not better things.

Clicked quickly around 14 times, and I have 14 empty blocks I do not need. Now it is 28 clicks to remove them all.

19 new (empty) blocks (after saving) for feeling around for literary about 60-120 seconds. Insane and crazy. What are you doing.

),
showInserter: ! isTyping( state ),

This comment has been minimized.

@aduth

aduth Apr 16, 2018

Member

I'm looking to enhance the between-inserter and am unclear the intent with this condition. Why do we only render the inserter button while not typing?

This comment has been minimized.

@youknowriad

youknowriad Apr 16, 2018

Contributor

Honestly, I don't remember why I added this exactly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment