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 showing move controls only for the selected block. #17315

Merged
merged 2 commits into from
Sep 16, 2019

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Sep 3, 2019

Followup from #17216. This PR experiments with showing the movers only for the selected block. This mirrors how this works on mobile (where there is no hover 🙂), and would close out issues like #16681 and #11725.

movers

I'm initially showing these all the time when the block is selected, but we can switch back to having them appear only when the left side of the block is hovered, if we'd prefer that.

@kjellr kjellr self-assigned this Sep 3, 2019
@kjellr kjellr added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. labels Sep 3, 2019
@shaunandrews
Copy link
Contributor

I absolutely love this.

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

This works well for solving the few related issues. It definitely highlights the difficulty of moving the Separator block which is already a known problem because now I need to click the block before moving it.

I don't mind the interaction at all. In fact, I'd say it causes me to focus on my actions much more purposefully which is a good thing.

I really like how you have the movers aligning with the block b/c they appear to be centered on the content when it's only one line.

Screen Shot 2019-09-03 at 2 01 50 PM

One little issue which isn't a blocker is how this overlaps the sibling inserter icon +. It's a bit funky and probably only related to the Separator block which is difficult anyways. So don't even worry about it.

Screen Shot 2019-09-03 at 2 02 34 PM

@jasmussen
Copy link
Contributor

Thanks for exploring this, I'll test this in a bit.

I want to note that if we merge this and decide we love it, there's some CSS refactoring we can do to the block structure itself. Right now there are some positive and negative paddings left and right of the blocks, which is there in part to ensure space left and right of the blocks, but also in part to make it so that hovering the block actually works. This can probably be simplified in an on-select only situation.

@ZebulanStanphill
Copy link
Member

I like this change.

As for the issue of the mover overlapping the appender button, I think that's really a fault of the appender button design. I think the appender really needs a redesign to fix all the various overlapping UI issues. See #13571.

@kjellr
Copy link
Contributor Author

kjellr commented Sep 4, 2019

Cool, thanks folks. Before merge, I'd love to get one more code review just to make sure I did this correctly and didn't rip out anything important by mistake. 😄

@mapk mapk removed the Needs Design Feedback Needs general design feedback. label Sep 6, 2019
@@ -49,7 +49,6 @@ import BlockInsertionPoint from './insertion-point';
import IgnoreNestedEvents from '../ignore-nested-events';
import InserterWithShortcuts from '../inserter-with-shortcuts';
import Inserter from '../inserter';
import useHoveredArea from './hover-area';
Copy link
Contributor

Choose a reason for hiding this comment

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

This hook is not used anymore, right? Can we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, good call! Removed in 40fbc51

@chrisvanpatten
Copy link
Contributor

I do think this should be merged, but would offer the personal caveat that I really like having movers available whether the block is selected or not (and I think I've advocated as such in the past). I think in a writing context, this workflow makes more sense but when you get into more "visual" page designs (for example, I'm working on building a custom gallery post type where each block represents a slide in the gallery) having the movers always-visible is much more useful.

That said, I think it's clear I'm in the minority; plus the idea of being able to refactor the block chrome CSS to be simpler in some circumstances (per @jasmussen's comment) would outweigh my own personal feelings on this one :)

@ZebulanStanphill
Copy link
Member

@chrisvanpatten I feel somewhat similar. If asked a year ago, I probably would have been against this PR since I like being able to move blocks without having to select them. But as I've had to deal with nested blocks more and more, the movers have become more and more distracting/annoying.

I now prefer the behavior in this PR. I think the positives outweigh the negatives in this case.

@kjellr
Copy link
Contributor Author

kjellr commented Sep 16, 2019

👋 We all seem to be mostly in agreement around this change, and the dev issues appear to be fixed. I'm going to go ahead and merge. Thanks folks!

@kjellr kjellr merged commit 074f51e into master Sep 16, 2019
@kjellr kjellr deleted the try/movers-only-on-select branch September 16, 2019 11:38
@kjellr kjellr added this to the Gutenberg 6.5 milestone Sep 16, 2019
@draganescu
Copy link
Contributor

Hi @kjellr did this close #16681 ?

@kjellr
Copy link
Contributor Author

kjellr commented Sep 25, 2019

@draganescu, yes! Thanks for the reminder. I'll close that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants