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: Restore block mover for floats. #12758

Merged
merged 3 commits into from
Feb 6, 2019
Merged

Try: Restore block mover for floats. #12758

merged 3 commits into from
Feb 6, 2019

Conversation

jasmussen
Copy link
Contributor

This PR hopes to fix #12736, #11424, #10300.

Currently we hide the block mover for any floated block. We do this because if you have a left floated image followed by a paragraph, the block mover for the image would overlap exactly with the block mover for the subsequent paragraph block. This would cause moving a float to be fiddly and virtually impossible as the hover states for the two conflicted.

This PR is an experiment to mitigate, or possibly fix that. It's a try branch because it is not necessarily a solid fix. What this PR does:

  • It makes it so the block mover cannot be invoked when only hovering a float, but when the float is selected they are permanently visible.
  • It changes some z-indexes so floats always have a higher z-index than not a float.
  • It changes the footprint of the side-UI container to "cover up" the underlying paragraph block, so it won't invoke unlesss you move the mouse below the footprint.

This is best explained in some GIFs. Here's one where the block mover footprint is coral colored to help explain what's going on:

float movers

Here's without that coral debug color:

without debug

The reason this is a "Try" branch is that when you move the mouse over the adjacent paragraph, the block mover is show as overlapping the selected floats block mover. But due to the rearranging of z-indexes, at least this is only a visual issue. A click on the floats block mover will still work as intended.

Why can't the float have on-hover block movers like every other block, you ask? Picture again the test-case of a left floated image followed by a paragraph of text. In this case, hovering over the image would show a block mover that would visually appear to be that of the paragraph block. By showing it when the float is selected, the context is clear.

We could experiment with not showing the hover block mover for float-adjacent blocks when hovered, but this isn't feasible because you might have a float, and then a block that clears this float, in which case the rule would break down.

Lay your thoughts on me.

@jasmussen jasmussen added the [Type] Enhancement A suggestion for improvement. label Dec 10, 2018
@jasmussen jasmussen self-assigned this Dec 10, 2018
{ isFirstMultiSelected && (
<BlockMultiControls rootClientId={ rootClientId } />
) }
<div className="editor-block-list__block-edit">
{ shouldRenderMovers && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the block mover to be able to work for a right-floated block, it has to be inside .editor-block-list__block-edit, or the control will show up way on the left of the main column. Same case as when we refactored the floats, essentially.

@@ -1,8 +1,8 @@
.editor-block-list__layout .components-draggable__clone {
// Hide the Block UI when dragging the block
// This ensures the page scroll properly (no sticky elements)
// Hide the Block UI when dragging the block.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a few comment cleanups while I'm in here.

@chrisvanpatten
Copy link
Contributor

I think the iteration could happen in a separate PR but I think this is another case where some version of #7211 would help with the mover overlap issues.

@jasmussen
Copy link
Contributor Author

I think the iteration could happen in a separate PR but I think this is another case where some version of #7211 would help with the mover overlap issues.

I know I've been skeptical in the past, but I'm starting to agree especially for nested blocks and now floats. I still don't really see the value it brings to top level blocks, except perhaps the consistency it would bring, but for those blocks which for now are still the most common, the extra chrome behind the controls just seems heavier.

@chrisvanpatten
Copy link
Contributor

@jasmussen Yeah it's primarily a consistency thing, although I do think it will be more practically valuable as the editor starts to reflect the front-end more and more and the "base level" editor canvas isn't so bare. But that could be a future iteration; it could definitely focus just on InnerBlocks for the moment.

@jasmussen jasmussen added this to the Future: Phase 2 milestone Jan 4, 2019
@jasmussen
Copy link
Contributor Author

@youknowriad For now I've set this in "future/phase 2". But I would appreciate a pair of eyes if you have time — could we get this into a plugin release like 4.9? I'd love for it to land early between major releases.

@youknowriad youknowriad modified the milestones: Future: Phase 2, 4.9 Jan 8, 2019
@youknowriad
Copy link
Contributor

I've put this PR in 4.9 to get some review 👀 It doesn't mean it's going to ship there though. Depends on the reviews.

@youknowriad youknowriad requested a review from a team January 8, 2019 08:11
@jasmussen
Copy link
Contributor Author

I'm not necessarily super antsy that it land in 4.9 or even 5.0, but I appreciate it being on the radar! Thanks Riad! 💕

.editor-block-contextual-toolbar {
// I think important is fine here to avoid over complexing the selector
// I think important is fine here to avoid over complexing the selector.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// I think important is fine here to avoid over complexing the selector.
// I think important is fine here to avoid over-complicating the selector.

@@ -168,7 +168,7 @@
background-color: $blue-medium-highlight;
}

// selection style for multiple blocks
// Selection style for multiple blocks.
&.is-multi-selected *::selection {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&.is-multi-selected *::selection {
&.is-multi-selected ::selection {

I think that asterisk is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought, but I'd prefer not actually making this change in this PR. It is already sort of big surgery and I'd like to minimize the changes as much as possible. Given that code did not change in this PR, only the comment, I'd prefer to look at this separately.

@jasmussen
Copy link
Contributor Author

Rebased, and addressed feedback. Tested again. I think this is good to go, it seems to work well for me. It would be good to get in after 5.1, but before 5.2, so I think now might be a good time to merge this.

One thing I fixed was hiding the block mover from the gray ghost when dragging:

dragging

@gziolo gziolo self-requested a review January 30, 2019 10:17
@youknowriad youknowriad removed this from the 5.0 (Gutenberg) milestone Feb 4, 2019
@gziolo gziolo deleted the try/float-mover-fix branch February 6, 2019 09:00
@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

Merged 🎉

@jasmussen
Copy link
Contributor Author

itshappening.rmvb!

youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Try: Restore block mover for floats.

This PR hopes to fix #12736, #11424, #10300.

Currently we hide the block mover for any floated block. We do this because if you have a left floated image followed by a paragraph, the block mover for the image would overlap exactly with the block mover for the subsequent paragraph block. This would cause moving a float to be fiddly and virtually impossible as the hover states for the two conflicted.

This PR is an experiment to mitigate, or possibly fix that. It's a try branch because it is not necessarily a solid fix. What this PR does:

- It makes it so the block mover cannot be invoked when only hovering a float, but when the float is selected they are permanently visible.
- It changes some z-indexes so floats always have a higher z-index than not a float.
- It changes the footprint of the side-UI container to "cover up" the underlying paragraph block, so it won't invoke unlesss you move the mouse below the footprint.

This is best explained in some GIFs.

The reason this is a "Try" branch is that when you move the mouse over the adjacent paragraph, the block mover is show as overlapping the selected floats block mover. But due to the rearranging of z-indexes, at least this is only a visual issue. A click on the floats block mover will still work as intended.

Why can't the float have on-hover block movers like every other block, you ask? Picture again the test-case of a left floated image followed by a paragraph of text. In this case, hovering over the image would show a block mover that would visually appear to be that of the paragraph block. By showing it when the float is selected, the context is clear.

We could experiment with not showing the hover block mover for float-adjacent blocks when hovered, but this isn't feasible because you might have a float, and then a block that clears this float, in which case the rule would break down.

Lay your thoughts on me.

* Address feedback, and improve dragging edgecase.

This addresses comment feedback by @ZebulanStanphill, thanks, and it also hides the block mover from the ghost when you are dragging.

* Add borders around floats.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Try: Restore block mover for floats.

This PR hopes to fix #12736, #11424, #10300.

Currently we hide the block mover for any floated block. We do this because if you have a left floated image followed by a paragraph, the block mover for the image would overlap exactly with the block mover for the subsequent paragraph block. This would cause moving a float to be fiddly and virtually impossible as the hover states for the two conflicted.

This PR is an experiment to mitigate, or possibly fix that. It's a try branch because it is not necessarily a solid fix. What this PR does:

- It makes it so the block mover cannot be invoked when only hovering a float, but when the float is selected they are permanently visible.
- It changes some z-indexes so floats always have a higher z-index than not a float.
- It changes the footprint of the side-UI container to "cover up" the underlying paragraph block, so it won't invoke unlesss you move the mouse below the footprint.

This is best explained in some GIFs.

The reason this is a "Try" branch is that when you move the mouse over the adjacent paragraph, the block mover is show as overlapping the selected floats block mover. But due to the rearranging of z-indexes, at least this is only a visual issue. A click on the floats block mover will still work as intended.

Why can't the float have on-hover block movers like every other block, you ask? Picture again the test-case of a left floated image followed by a paragraph of text. In this case, hovering over the image would show a block mover that would visually appear to be that of the paragraph block. By showing it when the float is selected, the context is clear.

We could experiment with not showing the hover block mover for float-adjacent blocks when hovered, but this isn't feasible because you might have a float, and then a block that clears this float, in which case the rule would break down.

Lay your thoughts on me.

* Address feedback, and improve dragging edgecase.

This addresses comment feedback by @ZebulanStanphill, thanks, and it also hides the block mover from the ghost when you are dragging.

* Add borders around floats.
@senadir
Copy link
Contributor

senadir commented Jun 3, 2019

Hi, this fix seems to cause a new bug #15923, #15930
setting the image block z-index to 81 overrides the block toolbar
this pull request should fix it #15966 while keeping everything right

This was referenced Apr 30, 2020
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.

Block mover hidden when left or right aligned
8 participants