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

Try a different inbetweenserter approach #6818

Merged
merged 5 commits into from
May 23, 2018

Conversation

jasmussen
Copy link
Contributor

This is an attempt to redesign the inbetweenserter to be less frustrating. Currently in master it's too easy to invoke it whe
n you don't mean to.

This branch changes that approach. You can still hover between two lines to reveal a line indicator. But you can't click this line. Instead, a new plus button sits at the center of this line, and you have to click that to insert between.

inbetweenserter

Please give this a spin. Depending on feedback, it needs visual love, and other tweaks. Right now it's definitely a try branch, and needs your thoughts. For example, when you click to insert a provisional placeholder block, you're still hovering in an area where the inserter line will appear, but it should immediately fade out instead. We might also want to look at some of that sizing magic that @aduth used for the old inbetweenserter.

@jasmussen jasmussen added the Needs Design Feedback Needs general design feedback. label May 18, 2018
@jasmussen jasmussen requested review from karmatosed, youknowriad, mtias, aduth and a team May 18, 2018 09:59
@chrisvanpatten
Copy link
Member

Bugs aside, this feels like a big improvement. Almost every time I compose content with Gutenberg I accidentally create extra blocks when I click the inbetweenserter, and in my brief testing this really mitigates that! Nice work. Excited to see it polished up.

Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

Whilst I know you're going to iterate I am approving just based on it being an improvement to what we have right now! Let's get this worked on and in as soon as possible.

@jasmussen
Copy link
Contributor Author

Yay! I see positive feedback and lots of hearts and approval. I'm going to do a polish pass on this, and see if we can fix the bugs.

@jasmussen jasmussen force-pushed the try/alternate-inbetweenserter branch from db12f2d to 71e568b Compare May 23, 2018 07:48
Joen Asmussen and others added 2 commits May 23, 2018 09:49
This is an attempt to redesign the inbetweenserter to be less frustrating. Currently in master it's too easy to invoke it whe
n you don't mean to.

This branch changes that approach. You can still hover between two lines to reveal a line indicator. But you can't click this line. Instead, a new plus button sits at the center of this line, and you have to click that to insert between.

Please give this a spin. Depending on feedback, it needs visual love, and other tweaks. Right now it's definitely a try branch, and needs your thoughts.
@jasmussen jasmussen force-pushed the try/alternate-inbetweenserter branch from 71e568b to 806e9ec Compare May 23, 2018 07:49
@jasmussen
Copy link
Contributor Author

jasmussen commented May 23, 2018

Rebased and squashed. Thanks to a contribution by Riad, it now looks like this:

updated

We should test that https://github.com/WordPress/gutenberg/pull/6818/files#diff-e0abfc1a3e466fbf4144905e29cf2ef5R22 doesn't cause any regressions. But otherwise, should we get this in early as Tammie suggests, and then look at separate iterations if need be?

icon="insert"
className="editor-block-list__insertion-point-button"
onClick={ this.onClick }
aria-label={ __( 'Insert block' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

aria-label doesn't work on the IconButton component, so this button is not labeled at all and announced just as "button". IconButton needs a label prop. That would also produce a tooltip, which is necessary to make the visually expose the name of the control.

Most importantly, the inserter doesn't show up on focus, when tabbing with the keyboard.

@afercia
Copy link
Contributor

afercia commented May 23, 2018

Using a label prop instead of aria-label makes the button properly labeled. It also adds a tooltip:

inserter

Haven't looked in depth to check why the inserter doesn't appear when focused with the keyboard, but that should be fixed 🙂

@jasmussen
Copy link
Contributor Author

Thanks, both Riad and Andrea! 🤘

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jasmussen jasmussen added this to the 3.0 milestone May 23, 2018
@jasmussen
Copy link
Contributor Author

Let's try this!

@jasmussen jasmussen merged commit 76e7b2a into master May 23, 2018
@jasmussen jasmussen deleted the try/alternate-inbetweenserter branch May 23, 2018 10:40
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.

5 participants