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

Spacer block: allow 1px height yet keep editor preview accuracy and improve selectability #25525

Conversation

stokesman
Copy link
Contributor

Welcome back to another iteration of how we might fix: #18906. Here the spacer block gets a minimum height of 1px in the frontend and 48px in the editor. In order to keep the preview inside the editor accurate, the spacer block will apply negative margins as appropriate:

spacer-min-height-margin-cheat

How has this been tested?

in Worpress 5.5.1 with Gutenberg plugin 9.0.0

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [n/a] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added [Block] Spacer Affects the Spacer Block [Type] Enhancement A suggestion for improvement. labels Sep 22, 2020
@stokesman
Copy link
Contributor Author

A less than ideal side effect is that the inserter/appender buttons shift along with the negative margins. Doesn't actually break anything but a bit wonky.
spacer-shifts-inserter

Also, it may be troublesome if a theme defines block margins of less than 24px as that would lead to overlaps of blocks not just their margins.

@talldan talldan added the Needs Design Feedback Needs general design feedback. label Sep 22, 2020
@talldan
Copy link
Contributor

talldan commented Sep 22, 2020

This is clever @stokesman.

I've added @mapk for some design feedback again and also @jasmussen this time, as I think Joen has a fair bit of knowledge of block margins.

@jasmussen
Copy link
Contributor

Wow what a journey. Thanks so much for your patience and creativity here, what's on display is quite profound. And high level this appears to work well. Before:

before

After:

after

It appears, as you describe, to work due to the negative margins:

negative margins

Also importantly, the unselected blocks appear to correctly render their heights fairly correctly and consistenly across frontend and backend. No spacer:

no spacer

Spacer with 1px height:

spacer 1px

Spacer with 20px height:

spacer 20px

This is also better than what's shipping in master. All the above is the good news.


The bad news is that the metrics aren't totally right, here the block is when multi selected:

spacer 20px metrics

The margins are still better than what's shipping, but at least the selected dimensions are correct there:

master metrics

I have concerns about the complexity of adding negative margins to account for a more fundamental issue of "every block being born with intrinsic margins" (see also #22208). Eventually I believe we'll actually remove those margins, because they are sort of arbitrary and don't really serve a purpose, and things will start to fall apart here.

For that reason, I tried two incredibly hacky versions of your branch here. Both branches remove that baseline block margin, but then go with and without negative margins.

With negative margins:

alt-with-negative-margins

The visuals obviously break apart a bit due to the nature of my hack. And at first glance, the effect looks correct, a 1px spacer only adds 1px of space. However you'll see in the preview that there's a disconnect with the frontend. Because the spacer, ironically, stops margins from collapsing. So where a margin: 1em 0; would collapse and only take up 1em between two paragraphs, when an element comes between them, suddenly there are 2em between them. Plus 1px.

In other words, the negative margins assume that there are always margins between blocks, because for a long time that's been the case in the editor. But it's not the case on the frontend, and as of #22209, it's not the case with groups. And unfortunately when you place a spacer with negative margins between two groups, things fall apart:

groups

The other hacky branch I tried was to remove the negative margins. Keeping in mind that in the following example I also removed all the baseline margins (#22208), and both the user experience and the frontend backend visuals are consistent and in sync.

alt-no-negative-margins

Notably this approach embraces margin collapsing. Sure that may not be the most obvious behavior for a user — insert a 1px spacer and it should only add 1px, right? But nevertheless this is how margin collapsing on the web works, and as shown above, if we don't embrace that behavior, things fall apart when the margins are zeroed out.


So, what's the next step?

I sense based on the opening phrase of this PR that allowing 1px height spacers without the negative margin trick has been tried before and dismissed, probably due to concerns with the sibling inserter and resizing UI. Nevertheless I'm going to investigate what we can do there, as I believe it to be the more resilient and stable approach. I'll ping on the PR.

@jasmussen
Copy link
Contributor

I created an alternate fix in #25528 that borrows a lot from this PR, but without the negative margins. Thank you @stokesman.

@stokesman
Copy link
Contributor Author

@jasmussen, I'm so glad that @talldan, added you on this. I had my doubts about this approach and your review was enlightening. We scored thanks to your investigation!

@talldan
Copy link
Contributor

talldan commented Sep 24, 2020

Closing as superseded by #25528, thanks again for helping get the eventual fix merged!

@talldan talldan closed this Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Spacer Affects the Spacer Block Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spacer Block: cannot adjust height to less than 20px
3 participants