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

Block Editor: Restore block movers to full-, wide-align blocks #15022

Merged
merged 7 commits into from
Apr 17, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 17, 2019

Fixes #14817
Related: #14145
Regression introduced in #12758

This pull request seeks to restore block movers for full- and wide-aligned blocks.

Implementation notes:

Reflected in the two commits, there were two necessary changes:

  • Try: Restore block mover for floats. #12758 had changed the markup structure to introduce a new div block-editor-block-list__block-edit. Many styles applied in block-list/style.scss used direct descendent selectors and were not updated to account for the new element.
  • Try using left borders for hover + selection states #14145 had made updates to the breadcrumb hover states which appeared to have assumed the absence of the block movers, and would otherwise conflict upon their reintroduction. I've proposed to reposition the hover label from the left to the right for wide/full-align blocks.

Note that this only updates the descendent selectors, but does not account for the fact that there are two possibilities for how a .block-editor-block-mover are rendered:

  • For multi-selections, it is still a direct descendent of .block-editor-block-list__block (source)
  • For singular selections, it is a descendent of the intermediary .block-editor-block-list__block-edit (source).

This was a result of the changes in #12758. It is unclear whether it requires further update to apply styles to both the direct descendent and the intermediary descendent.

There appears to be some issue which makes the movers harder to use. The cause is not yet determined, and I would not consider quite as blocking as the complete absence of movers, given timeline of WordPress 5.2 RC today.

Testing instructions:

Repeat Steps to Reproduce from #14817, verifying expected result.

@aduth aduth added [Priority] High Used to indicate top priority items that need quick attention [Package] Block editor /packages/block-editor labels Apr 17, 2019
@aduth aduth requested review from kjellr and jasmussen April 17, 2019 14:56
@aduth aduth requested a review from chrisvanpatten as a code owner April 17, 2019 14:56
@aduth
Copy link
Member Author

aduth commented Apr 17, 2019

cc @m-e-h . Apologies, I had meant to mark you as co-author since your debugging at #14817 (comment) was very accurate in leading to the underlying cause. I'd welcome your feedback if you have any.

@youknowriad
Copy link
Contributor

I'm seeing a small issue, where if I "hover" an unselected wide image, horizontal scrollbars appear. I don't know if this is specific to this PR yet but I don't see this as critical to fix (compared to the issue fixed by the PR itself)

@youknowriad
Copy link
Contributor

(The hover issue is specific to this branch)

float: left;
}

// Position hover label on the right
> .block-editor-block-list__breadcrumb {
> .block-editor-block-list__block-edit > .block-editor-block-list__breadcrumb {
right: -$border-width;
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a +1 px here solves the hover issue for me

Copy link
Member Author

@aduth aduth Apr 17, 2019

Choose a reason for hiding this comment

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

Adding a +1 px here solves the hover issue for me

I think it would effectively make it equivalent as right: 0; which sounds like what it should be (and conversely, the negative value could certainly explain the overflow). FWIW, I'm not able to reproduce the issue in Chrome, so I might imagine it's browser-specific.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs @kjellr input. Btw with the mover above, I think we could do with the hover label on the left again.

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.

Approving as everything seems fine in my tests aside the hover issue (to which I proposed a small fix)

@jasmussen
Copy link
Contributor

There was a regression with the toolbar on all wide blocks, it wasn't centered whereas it was in 4.7. This meant the movers weren't accessible on wide alignments. Fix pushed. Debugging with Andrew for the hover issue, and have some clues.

@jasmussen
Copy link
Contributor

Fixed the two issues that regressed -- sorry about that. The toolbar is centered on wide, so on the right breakpoints the movers are present again. And they are clickable, there was an inherit that targetted a rule no longer there. Thanks all.

@aduth
Copy link
Member Author

aduth commented Apr 17, 2019

I think this needs @kjellr input. Btw with the mover above, I think we could do with the hover label on the left again.

I restored the left-alignment in a combination of 93bdae6 (revert) and c2086aa (the "corrected" descendent selector interfered with the intended alignment of the hover label).

While I'd previously remarked about the potential for conflict here (#14145 (comment)), I think this is better due to:

@aduth
Copy link
Member Author

aduth commented Apr 17, 2019

The last commit 2b2e556 resolves my final uncertainty here by restoring descendent styling to multi-selections, since the descendent hierarchy differs between singular and multi-selections. Without these changes, a multi-selection which starts with a wide- or full-align block would still not show movers.

For all of the above, I would still recommend we explore in the future ways to consolidate these differences in markup / hierarchy, but this should be sufficient for resolving the immediate bug.

@youknowriad
Copy link
Contributor

youknowriad commented Apr 17, 2019

I think if we add e2e tests to move wide aligned and full-width aligned blocks we potentially guard against the two regressions seen here. (which I think is not the first time we see them regressing).

(Good as a follow-up though)

@aduth aduth merged commit dc58e1f into master Apr 17, 2019
@aduth aduth deleted the fix/14817-block-movers-full-width branch April 17, 2019 17:04
@aduth
Copy link
Member Author

aduth commented Apr 17, 2019

I'd accidentally left my merge settings as "Rebase and Merge" from a previous release, so the individual commits were merged rather than as a single unit. I will cherry-pick each individual commit into #15020 to retain this.

@kjellr
Copy link
Contributor

kjellr commented Apr 18, 2019

Sorry for coming into this late: I just want to follow up and check what I should be expecting to see here post-merge. When testing on my local install, I'm seeing the block movers return to wide and full-width blocks:

Screen Shot 2019-04-18 at 10 04 32 AM

I'm seeing overlap between the block label and the movers on hover. (As @aduth noted over in #14145 (comment), the placement of those labels seemed fine when the movers were mistakenly missing, but isn't great now). Is that what I should expect to see here? If so, I'll open a new PR to try addressing that.

Screen Shot 2019-04-18 at 10 02 01 AM

@aduth
Copy link
Member Author

aduth commented Apr 18, 2019

Thanks for checking in, @kjellr . All of what you mentioned is expected, yes.

I'd acknowledge and agree that the overlap is not great, but it is operable enough in my opinion to have been the best compromise to resolving the bug while respecting design for the immediate need of the WordPress 5.2 RC. Follow-up improvements would be welcomed, though it'd be my expectation that these would not be present for WordPress 5.2.

@kjellr
Copy link
Contributor

kjellr commented Apr 18, 2019

I'd acknowledge and agree that the overlap is not great, but it is operable enough in my opinion to have been the best compromise to resolving the bug while respecting design for the immediate need of the WordPress 5.2 RC. Follow-up improvements would be welcomed, though it'd be my expectation that these would not be present for WordPress 5.2.

Agreed. 👍 Thanks for the clarification. Since it's not a rush for 5.2, I'll work on getting a PR up for this early next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block mover controls hidden on full-width block
4 participants